From 6223d10d846d4cd2f27a14699a637d320f4d51a2 Mon Sep 17 00:00:00 2001 From: Hamilton Snow Date: Mon, 23 Mar 2026 20:35:11 +0800 Subject: [PATCH] fix(codex): native skills fallback refresh + legacy prompt suppression (#1930) * fix(codex): skip legacy prompts and fallback when bundled skills missing * fix(skills): allow native fallback to overwrite existing SKILL.md * fix(codex): defer legacy .codex cleanup until after skills fallback * fix(codex): preserve existing .codex while skipping legacy prompt extraction * docs(skills): clarify overwrite_existing behavior * test(codex): cover fresh-dir suppression of legacy .codex layout * docs(codex): clarify skip_legacy_codex_prompts suppresses full .codex dir * security(init): validate zip member paths before extraction --- src/specify_cli/__init__.py | 114 +++++++++++++++++++++++++++----- tests/test_ai_skills.py | 128 +++++++++++++++++++++++++++++++++--- 2 files changed, 215 insertions(+), 27 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index d2bf63ee..1e4c296e 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -948,9 +948,26 @@ def download_template_from_github(ai_assistant: str, download_dir: Path, *, scri } return zip_path, metadata -def download_and_extract_template(project_path: Path, ai_assistant: str, script_type: str, is_current_dir: bool = False, *, verbose: bool = True, tracker: StepTracker | None = None, client: httpx.Client = None, debug: bool = False, github_token: str = None) -> Path: +def download_and_extract_template( + project_path: Path, + ai_assistant: str, + script_type: str, + is_current_dir: bool = False, + *, + skip_legacy_codex_prompts: bool = False, + verbose: bool = True, + tracker: StepTracker | None = None, + client: httpx.Client = None, + debug: bool = False, + github_token: str = None, +) -> Path: """Download the latest release and extract it to create a new project. Returns project_path. Uses tracker if provided (with keys: fetch, download, extract, cleanup) + + Note: + ``skip_legacy_codex_prompts`` suppresses the legacy top-level + ``.codex`` directory from older template archives in Codex skills mode. + The name is kept for backward compatibility with existing callers. """ current_dir = Path.cwd() @@ -990,6 +1007,19 @@ def download_and_extract_template(project_path: Path, ai_assistant: str, script_ project_path.mkdir(parents=True) with zipfile.ZipFile(zip_path, 'r') as zip_ref: + def _validate_zip_members_within(root: Path) -> None: + """Validate all ZIP members stay within ``root`` (Zip Slip guard).""" + root_resolved = root.resolve() + for member in zip_ref.namelist(): + member_path = (root / member).resolve() + try: + member_path.relative_to(root_resolved) + except ValueError: + raise RuntimeError( + f"Unsafe path in ZIP archive: {member} " + "(potential path traversal)" + ) + zip_contents = zip_ref.namelist() if tracker: tracker.start("zip-list") @@ -1000,6 +1030,7 @@ def download_and_extract_template(project_path: Path, ai_assistant: str, script_ if is_current_dir: with tempfile.TemporaryDirectory() as temp_dir: temp_path = Path(temp_dir) + _validate_zip_members_within(temp_path) zip_ref.extractall(temp_path) extracted_items = list(temp_path.iterdir()) @@ -1019,6 +1050,11 @@ def download_and_extract_template(project_path: Path, ai_assistant: str, script_ console.print("[cyan]Found nested directory structure[/cyan]") for item in source_dir.iterdir(): + # In Codex skills mode, do not materialize the legacy + # top-level .codex directory from older prompt-based + # template archives. + if skip_legacy_codex_prompts and ai_assistant == "codex" and item.name == ".codex": + continue dest_path = project_path / item.name if item.is_dir(): if dest_path.exists(): @@ -1043,6 +1079,7 @@ def download_and_extract_template(project_path: Path, ai_assistant: str, script_ if verbose and not tracker: console.print("[cyan]Template files merged into current directory[/cyan]") else: + _validate_zip_members_within(project_path) zip_ref.extractall(project_path) extracted_items = list(project_path.iterdir()) @@ -1069,6 +1106,13 @@ def download_and_extract_template(project_path: Path, ai_assistant: str, script_ elif verbose: console.print("[cyan]Flattened nested directory structure[/cyan]") + # For fresh-directory Codex skills init, suppress legacy + # top-level .codex layout extracted from older archives. + if skip_legacy_codex_prompts and ai_assistant == "codex": + legacy_codex_dir = project_path / ".codex" + if legacy_codex_dir.is_dir(): + shutil.rmtree(legacy_codex_dir, ignore_errors=True) + except Exception as e: if tracker: tracker.error("extract", str(e)) @@ -1499,18 +1543,27 @@ def _get_skills_dir(project_path: Path, selected_ai: str) -> Path: return project_path / DEFAULT_SKILLS_DIR -def install_ai_skills(project_path: Path, selected_ai: str, tracker: StepTracker | None = None) -> bool: +def install_ai_skills( + project_path: Path, + selected_ai: str, + tracker: StepTracker | None = None, + *, + overwrite_existing: bool = False, +) -> bool: """Install Prompt.MD files from templates/commands/ as agent skills. Skills are written to the agent-specific skills directory following the `agentskills.io `_ specification. - Installation is additive — existing files are never removed and prompt - command files in the agent's commands directory are left untouched. + Installation is additive by default — existing files are never removed and + prompt command files in the agent's commands directory are left untouched. Args: project_path: Target project directory. selected_ai: AI assistant key from ``AGENT_CONFIG``. tracker: Optional progress tracker. + overwrite_existing: When True, overwrite any existing ``SKILL.md`` file + in the target skills directory (including user-authored content). + Defaults to False. Returns: ``True`` if at least one skill was installed or all skills were @@ -1640,9 +1693,10 @@ def install_ai_skills(project_path: Path, selected_ai: str, tracker: StepTracker skill_file = skill_dir / "SKILL.md" if skill_file.exists(): - # Do not overwrite user-customized skills on re-runs - skipped_count += 1 - continue + if not overwrite_existing: + # Default behavior: do not overwrite user-customized skills on re-runs + skipped_count += 1 + continue skill_file.write_text(skill_content, encoding="utf-8") installed_count += 1 @@ -1994,7 +2048,18 @@ def init( if use_github: with httpx.Client(verify=local_ssl_context) as local_client: - download_and_extract_template(project_path, selected_ai, selected_script, here, verbose=False, tracker=tracker, client=local_client, debug=debug, github_token=github_token) + download_and_extract_template( + project_path, + selected_ai, + selected_script, + here, + skip_legacy_codex_prompts=(selected_ai == "codex" and ai_skills), + verbose=False, + tracker=tracker, + client=local_client, + debug=debug, + github_token=github_token, + ) else: scaffold_ok = scaffold_from_core_pack(project_path, selected_ai, selected_script, here, tracker=tracker) if not scaffold_ok: @@ -2013,7 +2078,6 @@ def init( if not here and project_path.exists(): shutil.rmtree(project_path) raise typer.Exit(1) - # For generic agent, rename placeholder directory to user-specified path if selected_ai == "generic" and ai_commands_dir: placeholder_dir = project_path / ".speckit" / "commands" @@ -2033,16 +2097,30 @@ def init( if ai_skills: if selected_ai in NATIVE_SKILLS_AGENTS: skills_dir = _get_skills_dir(project_path, selected_ai) - if not _has_bundled_skills(project_path, selected_ai): - raise RuntimeError( - f"Expected bundled agent skills in {skills_dir.relative_to(project_path)}, " - "but none were found. Re-run with an up-to-date template." - ) - if tracker: - tracker.start("ai-skills") - tracker.complete("ai-skills", f"bundled skills → {skills_dir.relative_to(project_path)}") + bundled_found = _has_bundled_skills(project_path, selected_ai) + if bundled_found: + if tracker: + tracker.start("ai-skills") + tracker.complete("ai-skills", f"bundled skills → {skills_dir.relative_to(project_path)}") + else: + console.print(f"[green]✓[/green] Using bundled agent skills in {skills_dir.relative_to(project_path)}/") else: - console.print(f"[green]✓[/green] Using bundled agent skills in {skills_dir.relative_to(project_path)}/") + # Compatibility fallback: convert command templates to skills + # when an older template archive does not include native skills. + # This keeps `specify init --here --ai codex --ai-skills` usable + # in repos that already contain unrelated skills under .agents/skills. + fallback_ok = install_ai_skills( + project_path, + selected_ai, + tracker=tracker, + overwrite_existing=True, + ) + if not fallback_ok: + raise RuntimeError( + f"Expected bundled agent skills in {skills_dir.relative_to(project_path)}, " + "but none were found and fallback conversion failed. " + "Re-run with an up-to-date template." + ) else: skills_ok = install_ai_skills(project_path, selected_ai, tracker=tracker) diff --git a/tests/test_ai_skills.py b/tests/test_ai_skills.py index 0bccd48d..cf2b6b7b 100644 --- a/tests/test_ai_skills.py +++ b/tests/test_ai_skills.py @@ -11,10 +11,12 @@ Tests cover: """ import re +import zipfile import pytest import tempfile import shutil import yaml +import typer from pathlib import Path from unittest.mock import patch @@ -720,8 +722,8 @@ class TestNewProjectCommandSkip: mock_skills.assert_not_called() assert (target / ".agents" / "skills" / "speckit-specify" / "SKILL.md").exists() - def test_codex_native_skills_missing_fails_clearly(self, tmp_path): - """Codex native skills init should fail if bundled skills are missing.""" + def test_codex_native_skills_missing_falls_back_then_fails_cleanly(self, tmp_path): + """Codex should attempt fallback conversion when bundled skills are missing.""" from typer.testing import CliRunner runner = CliRunner() @@ -730,7 +732,7 @@ class TestNewProjectCommandSkip: with patch("specify_cli.download_and_extract_template", lambda *args, **kwargs: None), \ patch("specify_cli.ensure_executable_scripts"), \ patch("specify_cli.ensure_constitution_from_template"), \ - patch("specify_cli.install_ai_skills") as mock_skills, \ + patch("specify_cli.install_ai_skills", return_value=False) as mock_skills, \ patch("specify_cli.is_git_repo", return_value=False), \ patch("specify_cli.shutil.which", return_value="/usr/bin/codex"): result = runner.invoke( @@ -739,11 +741,13 @@ class TestNewProjectCommandSkip: ) assert result.exit_code == 1 - mock_skills.assert_not_called() + mock_skills.assert_called_once() + assert mock_skills.call_args.kwargs.get("overwrite_existing") is True assert "Expected bundled agent skills" in result.output + assert "fallback conversion failed" in result.output def test_codex_native_skills_ignores_non_speckit_skill_dirs(self, tmp_path): - """Non-spec-kit SKILL.md files should not satisfy Codex bundled-skills validation.""" + """Non-spec-kit SKILL.md files should trigger fallback conversion, not hard-fail.""" from typer.testing import CliRunner runner = CliRunner() @@ -757,7 +761,7 @@ class TestNewProjectCommandSkip: with patch("specify_cli.download_and_extract_template", side_effect=fake_download), \ patch("specify_cli.ensure_executable_scripts"), \ patch("specify_cli.ensure_constitution_from_template"), \ - patch("specify_cli.install_ai_skills") as mock_skills, \ + patch("specify_cli.install_ai_skills", return_value=True) as mock_skills, \ patch("specify_cli.is_git_repo", return_value=False), \ patch("specify_cli.shutil.which", return_value="/usr/bin/codex"): result = runner.invoke( @@ -765,9 +769,100 @@ class TestNewProjectCommandSkip: ["init", str(target), "--ai", "codex", "--ai-skills", "--script", "sh", "--no-git"], ) - assert result.exit_code == 1 - mock_skills.assert_not_called() - assert "Expected bundled agent skills" in result.output + assert result.exit_code == 0 + mock_skills.assert_called_once() + assert mock_skills.call_args.kwargs.get("overwrite_existing") is True + + def test_codex_ai_skills_here_mode_preserves_existing_codex_dir(self, tmp_path, monkeypatch): + """Codex --here skills init should not delete a pre-existing .codex directory.""" + from typer.testing import CliRunner + + runner = CliRunner() + target = tmp_path / "codex-preserve-here" + target.mkdir() + existing_prompts = target / ".codex" / "prompts" + existing_prompts.mkdir(parents=True) + (existing_prompts / "custom.md").write_text("custom") + monkeypatch.chdir(target) + + with patch("specify_cli.download_and_extract_template", return_value=target), \ + patch("specify_cli.ensure_executable_scripts"), \ + patch("specify_cli.ensure_constitution_from_template"), \ + patch("specify_cli.install_ai_skills", return_value=True), \ + patch("specify_cli.is_git_repo", return_value=True), \ + patch("specify_cli.shutil.which", return_value="/usr/bin/codex"): + result = runner.invoke( + app, + ["init", "--here", "--ai", "codex", "--ai-skills", "--script", "sh", "--no-git"], + input="y\n", + ) + + assert result.exit_code == 0 + assert (target / ".codex").exists() + assert (existing_prompts / "custom.md").exists() + + def test_codex_ai_skills_fresh_dir_does_not_create_codex_dir(self, tmp_path): + """Fresh-directory Codex skills init should not leave legacy .codex from archive.""" + target = tmp_path / "fresh-codex-proj" + archive = tmp_path / "codex-template.zip" + + with zipfile.ZipFile(archive, "w") as zf: + zf.writestr("template-root/.codex/prompts/speckit.specify.md", "legacy") + zf.writestr("template-root/.specify/templates/constitution-template.md", "constitution") + + fake_meta = { + "filename": archive.name, + "size": archive.stat().st_size, + "release": "vtest", + "asset_url": "https://example.invalid/template.zip", + } + + with patch("specify_cli.download_template_from_github", return_value=(archive, fake_meta)): + specify_cli.download_and_extract_template( + target, + "codex", + "sh", + is_current_dir=False, + skip_legacy_codex_prompts=True, + verbose=False, + ) + + assert target.exists() + assert (target / ".specify").exists() + assert not (target / ".codex").exists() + + @pytest.mark.parametrize("is_current_dir", [False, True]) + def test_download_and_extract_template_blocks_zip_path_traversal(self, tmp_path, monkeypatch, is_current_dir): + """Extraction should reject ZIP members escaping the target directory.""" + target = (tmp_path / "here-proj") if is_current_dir else (tmp_path / "new-proj") + if is_current_dir: + target.mkdir() + monkeypatch.chdir(target) + + archive = tmp_path / "malicious-template.zip" + with zipfile.ZipFile(archive, "w") as zf: + zf.writestr("../evil.txt", "pwned") + zf.writestr("template-root/.specify/templates/constitution-template.md", "constitution") + + fake_meta = { + "filename": archive.name, + "size": archive.stat().st_size, + "release": "vtest", + "asset_url": "https://example.invalid/template.zip", + } + + with patch("specify_cli.download_template_from_github", return_value=(archive, fake_meta)): + with pytest.raises(typer.Exit): + specify_cli.download_and_extract_template( + target, + "codex", + "sh", + is_current_dir=is_current_dir, + skip_legacy_codex_prompts=True, + verbose=False, + ) + + assert not (tmp_path / "evil.txt").exists() def test_commands_preserved_when_skills_fail(self, tmp_path): """If skills fail, commands should NOT be removed (safety net).""" @@ -859,6 +954,21 @@ class TestSkipIfExists: # All 4 templates should produce skills (specify, plan, tasks, empty_fm) assert len(skill_dirs) == 4 + def test_existing_skill_overwritten_when_enabled(self, project_dir, templates_dir): + """When overwrite_existing=True, pre-existing SKILL.md should be replaced.""" + skill_dir = project_dir / ".claude" / "skills" / "speckit-specify" + skill_dir.mkdir(parents=True) + custom_content = "# My Custom Specify Skill\nUser-modified content\n" + skill_file = skill_dir / "SKILL.md" + skill_file.write_text(custom_content) + + result = install_ai_skills(project_dir, "claude", overwrite_existing=True) + + assert result is True + updated_content = skill_file.read_text() + assert updated_content != custom_content + assert "name: speckit-specify" in updated_content + # ===== SKILL_DESCRIPTIONS Coverage Tests =====