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

Author: RbBtSn0w

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 <https://agentskills.io/specification>`_ 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)
 

tests/test_ai_skills.py

@@ -11,10 +11,12 @@
 """
 
 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 @@ def fake_download(project_path, *args, **kwargs):
         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 @@ def test_codex_native_skills_missing_fails_clearly(self, tmp_path):
         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 @@ def test_codex_native_skills_missing_fails_clearly(self, tmp_path):
             )
 
         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,17 +761,108 @@ def fake_download(project_path, *args, **kwargs):
         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(
                 app,
                 ["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 @@ def test_fresh_install_writes_all_skills(self, project_dir, templates_dir):
         # 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 =====