From 8c4c5bb7713f773e5c3c90fc7931de9a9cf1b0bd Mon Sep 17 00:00:00 2001 From: Ed Harrod Date: Sun, 25 Oct 2026 18:39:21 +0000 Subject: [PATCH 1/4] feat(codex): add --no-model-invocation integration option Refs #2654. Writes agents/openai.yaml with policy.allow_implicit_invocation: false for core command skills; extension skills stay model-invocable. Assisted-by: Claude Code (claude-sonnet-5-5) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Xh3oZ7vsiPpGXAqtZnULC8 --- docs/integrations/codex.md | 36 ++++- .../integrations/codex/__init__.py | 68 +++++++++ tests/integrations/test_integration_codex.py | 133 ++++++++++++++++++ 3 files changed, 236 insertions(+), 1 deletion(-) diff --git a/docs/integrations/codex.md b/docs/integrations/codex.md index d37b6901f5..7eecc2256c 100644 --- a/docs/integrations/codex.md +++ b/docs/integrations/codex.md @@ -26,10 +26,44 @@ Other integrations also target `.agents/skills/`; review [shared multi-install rules](cli.md#can-i-install-multiple-integrations-in-the-same-project) before installing another agent that shares this directory. +## User-Invocable-Only Skills + +To stop Codex invoking the core Spec Kit command skills implicitly: + +```bash +specify init my-project --integration codex --integration-options="--no-model-invocation" +``` + +This writes `agents/openai.yaml` with `policy.allow_implicit_invocation: false` +next to each core command skill. OpenAI's +[skills documentation](https://learn.chatgpt.com/docs/build-skills) says Codex +then does not invoke the skill implicitly from the prompt, and explicit `$skill` +invocation still works. It does not say whether such a skill keeps its name and +description in Codex's context. + +- Extension skills stay model-invocable, because mandatory extension hooks rely + on the model invoking them. If an extension's mandatory hook runs a core + command (its `command:` is a core `speckit.`), leave this option off: + the model cannot run that hook while the core skill is user-invocable only. +- The option is stored with the integration's other options and reapplied by + `specify integration upgrade codex`. Passing `--integration-options` to + `upgrade` replaces all stored options (to keep both, pass + `--integration-options="--events false --no-model-invocation"`); + `--integration-options=""` clears them and removes the generated `agents/openai.yaml` files. Re-running + `specify init` likewise stores only the options passed to it. +- An existing `agents/openai.yaml` keeps its other keys; only + `policy.allow_implicit_invocation` is set. Skills that commands other than + `init`, `install` and `upgrade` register (for example presets that add core + skills) get no `agents/openai.yaml` until `specify integration upgrade codex + --force` is run. +- Preset overrides rewrite only `SKILL.md`, so the sibling `agents/openai.yaml` + of an overridden core skill is left in place. + ## Options and Workflow Dispatch `--integration-options="--skills"` explicitly selects the skills layout, which -is already the only supported layout for this integration. Native events use +is already the only supported layout for this integration, and +`--no-model-invocation` is described above. Native events use `.codex/config.toml`; `--events false` disables Spec Kit event handlers. See [runtime events](cli.md#runtime-events). diff --git a/src/specify_cli/integrations/codex/__init__.py b/src/specify_cli/integrations/codex/__init__.py index d88628b3ff..e90fca5861 100644 --- a/src/specify_cli/integrations/codex/__init__.py +++ b/src/specify_cli/integrations/codex/__init__.py @@ -10,7 +10,10 @@ from collections.abc import Mapping, Sequence from typing import Any +import yaml + from ..base import IntegrationOption, SkillsIntegration +from ..manifest import IntegrationManifest class CodexIntegration(SkillsIntegration): @@ -44,6 +47,60 @@ class CodexIntegration(SkillsIntegration): events_config_file = ".codex/config.toml" events_format = "toml" + @staticmethod + def _openai_yaml_with_policy(path: Path) -> str: + """Return ``path``'s YAML with implicit invocation disabled, keeping its other keys.""" + data: dict[str, Any] = {} + if path.is_file(): + try: + loaded = yaml.safe_load(path.read_text(encoding="utf-8")) + except (yaml.YAMLError, OSError, UnicodeDecodeError): + loaded = None + if isinstance(loaded, dict): + data = loaded + policy = data.get("policy") + if not isinstance(policy, dict): + policy = {} + policy["allow_implicit_invocation"] = False + data["policy"] = policy + return yaml.safe_dump(data, sort_keys=False) + + @staticmethod + def _is_core_command_skill_dir(name: str) -> bool: + from specify_cli.extensions import CORE_COMMAND_NAMES # lazy: avoids circular import + + return name.startswith("speckit-") and ( + name.removeprefix("speckit-") in CORE_COMMAND_NAMES + ) + + def setup( + self, + project_root: Path, + manifest: IntegrationManifest, + parsed_options: dict[str, Any] | None = None, + **opts: Any, + ) -> list[Path]: + created = super().setup( + project_root, manifest, parsed_options=parsed_options, **opts + ) + if (parsed_options or {}).get("no_model_invocation") is not True: + return created + # Codex has no SKILL.md frontmatter flag; the policy lives in a sibling + # ``agents/openai.yaml``. It is written for core command skills only, + # since mandatory extension hooks rely on the model invoking them. + for skill_file in [f for f in created if f.name == "SKILL.md"]: + if self._is_core_command_skill_dir(skill_file.parent.name): + dest = skill_file.parent / "agents" / "openai.yaml" + created.append( + self.write_file_and_record( + self._openai_yaml_with_policy(dest), + dest, + project_root, + manifest, + ) + ) + return created + def build_exec_args( self, prompt: str, @@ -77,4 +134,15 @@ def options(cls) -> list[IntegrationOption]: help="Install as agent skills (default for Codex)", ) ) + opts.append( + IntegrationOption( + "--no-model-invocation", + is_flag=True, + default=False, + help=( + "Install core command skills as user-invocable only " + "(agents/openai.yaml: allow_implicit_invocation: false)" + ), + ) + ) return opts diff --git a/tests/integrations/test_integration_codex.py b/tests/integrations/test_integration_codex.py index 9a4dc5442b..327fcc5166 100644 --- a/tests/integrations/test_integration_codex.py +++ b/tests/integrations/test_integration_codex.py @@ -1,10 +1,14 @@ """Tests for CodexIntegration.""" +from pathlib import Path + from specify_cli.integrations import get_integration from specify_cli.integrations.manifest import IntegrationManifest from .test_integration_base_skills import SkillsIntegrationTests +REPO_ROOT = Path(__file__).resolve().parents[2] + class TestCodexIntegration(SkillsIntegrationTests): KEY = "codex" @@ -191,3 +195,132 @@ def test_hook_note_when_instruction_is_final_line_without_newline(self): assert "For each executable hook" not in lines[note_line_idx], ( "Note and instruction must not be on the same line" ) + + +class TestCodexNoModelInvocationOption: + """``--integration-options="--no-model-invocation"`` opts core skills out of implicit invocation (#2654).""" + + @staticmethod + def _policy(skill_dir): + import yaml + + return yaml.safe_load((skill_dir / "agents" / "openai.yaml").read_text(encoding="utf-8")) + + def test_options_include_no_model_invocation_flag(self): + opts = get_integration("codex").options() + matching = [o for o in opts if o.name == "--no-model-invocation"] + assert len(matching) == 1 + assert matching[0].is_flag is True + assert matching[0].default is False + + def test_setup_with_option_writes_policy_for_core_skills(self, tmp_path): + i = get_integration("codex") + m = IntegrationManifest("codex", tmp_path) + created = i.setup( + tmp_path, m, parsed_options={"no_model_invocation": True}, 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: + assert self._policy(f.parent) == {"policy": {"allow_implicit_invocation": False}}, f.parent.name + yamls = [f for f in created if f.name == "openai.yaml"] + assert len(yamls) == len(skill_files) + assert all( + y.relative_to(tmp_path).as_posix() in m.files for y in yamls + ) + + def test_setup_without_option_writes_no_policy(self, tmp_path): + i = get_integration("codex") + created = i.setup(tmp_path, IntegrationManifest("codex", tmp_path), script_type="sh") + assert not [f for f in created if f.name == "openai.yaml"] + assert not list((tmp_path / ".agents" / "skills").glob("*/agents")) + + def test_extension_named_skill_dirs_are_not_core(self): + i = get_integration("codex") + assert i._is_core_command_skill_dir("speckit-plan") + assert not i._is_core_command_skill_dir("speckit-git-feature") + assert not i._is_core_command_skill_dir("plan") + + def test_cli_rejects_value_for_flag(self, tmp_path): + import os + + from typer.testing import CliRunner + from specify_cli import app + + project = tmp_path / "codex-bad-option" + project.mkdir() + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke( + app, + ["init", "--here", "--integration", "codex", "--integration-options", + "--no-model-invocation=yes", "--script", "sh", "--ignore-agent-tools"], + catch_exceptions=False, + ) + finally: + os.chdir(old_cwd) + assert result.exit_code != 0 + assert "does not accept a value" in result.output + assert not (project / ".agents" / "skills").exists() + + def test_option_persists_across_upgrade_and_clearing_removes_policy(self, tmp_path): + import json + + from tests.specify_cli.integrations._helpers import _init_project, _run_in_project + + project = _init_project(tmp_path, "codex", "--no-model-invocation") + skills_dir = project / ".agents" / "skills" + policies = sorted(skills_dir.glob("*/agents/openai.yaml")) + assert len(policies) > 0 + state = json.loads((project / ".specify" / "integration.json").read_text(encoding="utf-8")) + assert state["integration_settings"]["codex"]["parsed_options"] == {"no_model_invocation": True} + + result = _run_in_project(project, ["integration", "upgrade", "codex", "--force"]) + assert result.exit_code == 0, result.output + assert sorted(skills_dir.glob("*/agents/openai.yaml")) == policies + + result = _run_in_project( + project, + ["integration", "upgrade", "codex", "--force", "--integration-options", ""], + ) + assert result.exit_code == 0, result.output + assert not list(skills_dir.glob("*/agents/openai.yaml")) + manifest = (project / ".specify" / "integrations" / "codex.manifest.json").read_text(encoding="utf-8") + assert "openai.yaml" not in manifest + + def test_extension_skills_have_no_policy_after_upgrade(self, tmp_path): + from tests.specify_cli.integrations._helpers import _init_project, _run_in_project + + project = _init_project(tmp_path, "codex", "--no-model-invocation") + result = _run_in_project( + project, ["extension", "add", "--dev", str(REPO_ROOT / "extensions" / "git")] + ) + assert result.exit_code == 0, result.output + result = _run_in_project(project, ["integration", "upgrade", "codex", "--force"]) + assert result.exit_code == 0, result.output + skills_dir = project / ".agents" / "skills" + git_skills = sorted(skills_dir.glob("speckit-git-*/SKILL.md")) + assert git_skills, "git extension skills were not generated" + for f in git_skills: + assert not (f.parent / "agents").exists(), f.parent.name + assert (skills_dir / "speckit-specify" / "agents" / "openai.yaml").exists() + + def test_existing_openai_yaml_keys_are_preserved(self, tmp_path): + import yaml + + existing = tmp_path / ".agents" / "skills" / "speckit-plan" / "agents" / "openai.yaml" + existing.parent.mkdir(parents=True) + existing.write_text("interface:\n display_name: Plan\n", encoding="utf-8") + i = get_integration("codex") + i.setup( + tmp_path, + IntegrationManifest("codex", tmp_path), + parsed_options={"no_model_invocation": True}, + script_type="sh", + ) + data = yaml.safe_load(existing.read_text(encoding="utf-8")) + assert data == { + "interface": {"display_name": "Plan"}, + "policy": {"allow_implicit_invocation": False}, + } From c409bf2bf31422fed10a5a5e1790aa64d53faeb9 Mon Sep 17 00:00:00 2001 From: Ed Harrod Date: Wed, 28 Oct 2026 10:39:07 +0000 Subject: [PATCH 2/4] fix(codex): preserve user openai.yaml and reconcile policies on disable Setup no longer overwrites an unreadable, invalid or non-mapping agents/openai.yaml: it validates every target before writing any and raises naming the file. Policy files are written with record_file() on the lexical project-relative path, so a symlinked file or agents/ directory is refused before anything is written outside the project. A pre-existing or user-edited openai.yaml is recorded as partial and keeps that ownership across upgrades; uninstall, clearing the option and re-running init without it remove only policy.allow_implicit_invocation from such files, and delete only unmodified generated ones. Setup reconciles against the prior on-disk manifest, so re-init without the option no longer leaves implicit invocation disabled. A user file that already sets the policy is not claimed. Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015w5WirGUspLsYEpCkaHWxV --- docs/integrations/codex.md | 14 +- .../integrations/codex/__init__.py | 185 ++++++++++++++--- tests/integrations/test_integration_codex.py | 195 ++++++++++++++++++ 3 files changed, 362 insertions(+), 32 deletions(-) diff --git a/docs/integrations/codex.md b/docs/integrations/codex.md index 7eecc2256c..dbfa693e65 100644 --- a/docs/integrations/codex.md +++ b/docs/integrations/codex.md @@ -49,10 +49,16 @@ description in Codex's context. `specify integration upgrade codex`. Passing `--integration-options` to `upgrade` replaces all stored options (to keep both, pass `--integration-options="--events false --no-model-invocation"`); - `--integration-options=""` clears them and removes the generated `agents/openai.yaml` files. Re-running - `specify init` likewise stores only the options passed to it. -- An existing `agents/openai.yaml` keeps its other keys; only - `policy.allow_implicit_invocation` is set. Skills that commands other than + `--integration-options=""` clears them and undoes the policy. Re-running + `specify init` likewise stores only the options passed to it, and re-running + it without the option undoes the policy too. +- Undoing the policy (clearing the option or `specify integration uninstall + codex`) deletes the `agents/openai.yaml` files Spec Kit generated. An + `agents/openai.yaml` that already existed, or that you edited, keeps its other + keys: only `policy.allow_implicit_invocation` is set, and later removed again. + One that already sets it to `false` is left untouched. If an existing file + cannot be read, is not valid YAML, or is not a mapping, setup stops with an + error naming the file and leaves it unchanged. Skills that commands other than `init`, `install` and `upgrade` register (for example presets that add core skills) get no `agents/openai.yaml` until `specify integration upgrade codex --force` is run. diff --git a/src/specify_cli/integrations/codex/__init__.py b/src/specify_cli/integrations/codex/__init__.py index e90fca5861..5a8e4a799c 100644 --- a/src/specify_cli/integrations/codex/__init__.py +++ b/src/specify_cli/integrations/codex/__init__.py @@ -5,15 +5,20 @@ """ from __future__ import annotations -from pathlib import Path +import hashlib +import logging from collections.abc import Mapping, Sequence +from pathlib import Path, PurePosixPath from typing import Any import yaml +from .._file_changes import write_bytes from ..base import IntegrationOption, SkillsIntegration -from ..manifest import IntegrationManifest +from ..manifest import IntegrationManifest, _validate_record_path + +logger = logging.getLogger(__name__) class CodexIntegration(SkillsIntegration): @@ -48,22 +53,127 @@ class CodexIntegration(SkillsIntegration): events_format = "toml" @staticmethod - def _openai_yaml_with_policy(path: Path) -> str: - """Return ``path``'s YAML with implicit invocation disabled, keeping its other keys.""" - data: dict[str, Any] = {} - if path.is_file(): - try: - loaded = yaml.safe_load(path.read_text(encoding="utf-8")) - except (yaml.YAMLError, OSError, UnicodeDecodeError): - loaded = None - if isinstance(loaded, dict): - data = loaded - policy = data.get("policy") - if not isinstance(policy, dict): - policy = {} + def _load_openai_yaml(path: Path, rel: str) -> dict[str, Any]: + """Parse an existing ``openai.yaml``; raise rather than guess at unusable content.""" + try: + loaded = yaml.safe_load(path.read_text(encoding="utf-8")) + except (yaml.YAMLError, OSError, UnicodeDecodeError) as exc: + raise ValueError(f"Cannot update {rel}: {exc}") from exc + if loaded is None: # an empty file holds nothing to preserve + return {} + if not isinstance(loaded, dict): + raise ValueError( + f"Cannot update {rel}: expected a YAML mapping, got {type(loaded).__name__}" + ) + policy = loaded.get("policy") + if policy is not None and not isinstance(policy, dict): + raise ValueError( + f"Cannot update {rel}: 'policy' must be a mapping, got {type(policy).__name__}" + ) + return loaded + + def _is_policy_rel(self, rel: str) -> bool: + parts = PurePosixPath(rel).parts + skills = PurePosixPath(self.registrar_config["dir"]).parts + return ( + len(parts) == len(skills) + 3 + and parts[: len(skills)] == skills + and parts[-2:] == ("agents", "openai.yaml") + ) + + def _prior_manifest(self, project_root: Path) -> IntegrationManifest | None: + """Return the on-disk manifest, which setup's callers save only afterwards.""" + try: + return IntegrationManifest.load(self.key, project_root) + except FileNotFoundError: + return None + except (OSError, ValueError) as exc: + logger.warning( + "Could not read prior manifest for '%s'; leaving earlier " + "agents/openai.yaml policies in place: %s", + self.key, exc, + ) + return None + + @staticmethod + def _matches_recorded(path: Path, manifest: IntegrationManifest, rel: str) -> bool: + try: + return hashlib.sha256(path.read_bytes()).hexdigest() == manifest.files.get(rel) + except OSError: + return False + + def _plan_policy( + self, project_root: Path, rel: str, prior: IntegrationManifest | None + ) -> tuple[str, str, str] | None: + """Return ``(rel, content, ownership)`` to write, or ``None`` to leave a user file alone.""" + path = project_root / rel + # Checks every ancestor for symlinks before anything is read or written. + _validate_record_path(Path(rel), project_root) + tracked = prior is not None and rel in prior.files + if not path.exists(): + data: dict[str, Any] = {} + ownership = "whole" + elif not path.is_file(): + raise ValueError(f"Cannot update {rel}: not a regular file") + else: + data = self._load_openai_yaml(path, rel) + generated = ( + tracked + and prior.ownership_modes[rel] == "whole" + and self._matches_recorded(path, prior, rel) + ) + ownership = "whole" if generated else "partial" + policy = data.get("policy") or {} + if ownership == "partial" and not tracked and policy.get("allow_implicit_invocation") is False: + # The user already set it; claiming it would let cleanup remove their setting. + return None policy["allow_implicit_invocation"] = False data["policy"] = policy - return yaml.safe_dump(data, sort_keys=False) + return rel, yaml.safe_dump(data, sort_keys=False), ownership + + def _remove_policy(self, project_root: Path, rel: str) -> bool: + """Drop the managed policy from a user-owned file; return whether the file remains.""" + path = project_root / rel + try: + _validate_record_path(Path(rel), project_root) + if not path.is_file(): + return False + data = self._load_openai_yaml(path, rel) + except (OSError, ValueError) as exc: + logger.warning("Leaving %s unchanged: %s", rel, exc) + return False + policy = data.get("policy") + if isinstance(policy, dict) and policy.get("allow_implicit_invocation") is False: + del policy["allow_implicit_invocation"] + if not policy: + del data["policy"] + write_bytes(path, yaml.safe_dump(data, sort_keys=False).encode("utf-8")) + return True + + def _release_prior_policies( + self, + project_root: Path, + manifest: IntegrationManifest, + prior: IntegrationManifest, + keep: set[str], + ) -> None: + """Undo policies an earlier install wrote that this setup no longer writes. + + Unmodified generated files are deleted; in user files only the policy is removed. + """ + generated = IntegrationManifest(self.key, project_root, version="policy-cleanup") + modes = prior.ownership_modes + for rel, recorded_hash in prior.files.items(): + if rel in keep or not self._is_policy_rel(rel): + continue + path = project_root / rel + if modes[rel] == "whole" and self._matches_recorded(path, prior, rel): + generated._files[rel] = recorded_hash + elif self._remove_policy(project_root, rel) and modes[rel] == "whole": + # A generated file the user edited is theirs now. Track it as + # partial so upgrade's stale-file pass does not delete it. + manifest.record_existing(rel, ownership="partial") + generated.uninstall(project_root, remove_manifest=False) @staticmethod def _is_core_command_skill_dir(name: str) -> bool: @@ -83,24 +193,43 @@ def setup( created = super().setup( project_root, manifest, parsed_options=parsed_options, **opts ) - if (parsed_options or {}).get("no_model_invocation") is not True: - return created # Codex has no SKILL.md frontmatter flag; the policy lives in a sibling # ``agents/openai.yaml``. It is written for core command skills only, # since mandatory extension hooks rely on the model invoking them. - for skill_file in [f for f in created if f.name == "SKILL.md"]: - if self._is_core_command_skill_dir(skill_file.parent.name): - dest = skill_file.parent / "agents" / "openai.yaml" - created.append( - self.write_file_and_record( - self._openai_yaml_with_policy(dest), - dest, - project_root, - manifest, + # Every file is validated before any is written. + planned: list[tuple[str, str, str]] = [] + prior = self._prior_manifest(project_root) + if (parsed_options or {}).get("no_model_invocation") is True: + for skill_file in [f for f in created if f.name == "SKILL.md"]: + if self._is_core_command_skill_dir(skill_file.parent.name): + dest = skill_file.parent / "agents" / "openai.yaml" + plan = self._plan_policy( + project_root, dest.relative_to(project_root).as_posix(), prior ) - ) + if plan is not None: + planned.append(plan) + if prior is not None: + self._release_prior_policies( + project_root, manifest, prior, {rel for rel, _, _ in planned} + ) + for rel, content, ownership in planned: + created.append(manifest.record_file(rel, content, ownership=ownership)) return created + def teardown( + self, + project_root: Path, + manifest: IntegrationManifest, + *, + force: bool = False, + ) -> tuple[list[Path], list[Path]]: + # ``manifest.uninstall`` never touches partial files, so remove the + # managed policy from user-owned ``openai.yaml`` files here. + for rel, mode in manifest.ownership_modes.items(): + if mode != "whole" and self._is_policy_rel(rel): + self._remove_policy(project_root, rel) + return super().teardown(project_root, manifest, force=force) + def build_exec_args( self, prompt: str, diff --git a/tests/integrations/test_integration_codex.py b/tests/integrations/test_integration_codex.py index 327fcc5166..5313c86ffe 100644 --- a/tests/integrations/test_integration_codex.py +++ b/tests/integrations/test_integration_codex.py @@ -1,7 +1,10 @@ """Tests for CodexIntegration.""" +import os from pathlib import Path +import pytest + from specify_cli.integrations import get_integration from specify_cli.integrations.manifest import IntegrationManifest @@ -324,3 +327,195 @@ def test_existing_openai_yaml_keys_are_preserved(self, tmp_path): "interface": {"display_name": "Plan"}, "policy": {"allow_implicit_invocation": False}, } + + +class TestCodexNoModelInvocationOwnership: + """``agents/openai.yaml`` handling must never destroy user configuration (#2654).""" + + PLAN_REL = ".agents/skills/speckit-plan/agents/openai.yaml" + POLICY = {"allow_implicit_invocation": False} + + @staticmethod + def _setup(project, enabled=True, manifest=None): + manifest = manifest or IntegrationManifest("codex", project) + get_integration("codex").setup( + project, + manifest, + parsed_options={"no_model_invocation": True} if enabled else None, + script_type="sh", + ) + return manifest + + @staticmethod + def _init(project, *options): + from tests.specify_cli.integrations._helpers import _run_in_project + + args = ["init", "--here", "--force", "--integration", "codex", + "--script", "sh", "--ignore-agent-tools"] + if options: + args += ["--integration-options", " ".join(options)] + result = _run_in_project(project, args) + assert result.exit_code == 0, result.output + return result + + @staticmethod + def _manifest(project): + return IntegrationManifest.load("codex", project) + + @staticmethod + def _yaml(path): + import yaml + + return yaml.safe_load(path.read_text(encoding="utf-8")) + + def _seed(self, project, content=b"interface:\n display_name: Plan\n"): + path = project / self.PLAN_REL + path.parent.mkdir(parents=True) + path.write_bytes(content) + return path + + def _other_policies(self, project): + return [ + p for p in (project / ".agents" / "skills").glob("*/agents/openai.yaml") + if p.relative_to(project).as_posix() != self.PLAN_REL + ] + + @pytest.mark.parametrize( + "content", + [ + b"interface: [unclosed\n", + b"- not\n- a mapping\n", + b"policy: not-a-mapping\n", + b"\xff\xfe\x00 invalid utf-8\n", + ], + ids=["invalid-yaml", "list-root", "scalar-policy", "invalid-utf8"], + ) + def test_unusable_existing_file_aborts_without_overwriting(self, tmp_path, content): + path = self._seed(tmp_path, content) + with pytest.raises(ValueError, match="speckit-plan/agents/openai.yaml"): + self._setup(tmp_path) + assert path.read_bytes() == content + assert self._other_policies(tmp_path) == [] + + def test_unreadable_existing_file_aborts_without_overwriting(self, tmp_path, monkeypatch): + path = self._seed(tmp_path) + original = path.read_bytes() + real_read_text = Path.read_text + + def read_text(self, *args, **kwargs): + if self.name == "openai.yaml": + raise PermissionError("denied") + return real_read_text(self, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", read_text) + with pytest.raises(ValueError, match="speckit-plan/agents/openai.yaml"): + self._setup(tmp_path) + monkeypatch.undo() + assert path.read_bytes() == original + + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + @pytest.mark.parametrize("linked", ["file", "agents-dir"]) + def test_symlink_outside_project_is_not_written(self, tmp_path, linked): + project = tmp_path / "proj" + outside = tmp_path / "outside" + outside.mkdir() + external = outside / "openai.yaml" + external.write_bytes(b"interface:\n display_name: External\n") + agents_dir = project / ".agents" / "skills" / "speckit-plan" / "agents" + if linked == "file": + agents_dir.mkdir(parents=True) + (agents_dir / "openai.yaml").symlink_to(external) + else: + agents_dir.parent.mkdir(parents=True) + agents_dir.symlink_to(outside, target_is_directory=True) + with pytest.raises(ValueError, match="symlink"): + self._setup(project) + assert external.read_bytes() == b"interface:\n display_name: External\n" + + def test_existing_user_file_is_partial_and_keeps_its_keys(self, tmp_path): + path = self._seed(tmp_path) + manifest = self._setup(tmp_path) + assert self._yaml(path) == { + "interface": {"display_name": "Plan"}, + "policy": self.POLICY, + } + assert manifest.ownership_modes[self.PLAN_REL] == "partial" + generated = self._other_policies(tmp_path)[0].relative_to(tmp_path).as_posix() + assert manifest.ownership_modes[generated] == "whole" + + def test_user_file_already_disabling_implicit_invocation_is_not_claimed(self, tmp_path): + content = b"policy:\n allow_implicit_invocation: false\n" + path = self._seed(tmp_path, content) + manifest = self._setup(tmp_path) + assert self.PLAN_REL not in manifest.files + get_integration("codex").teardown(tmp_path, manifest) + assert path.read_bytes() == content + + def test_teardown_removes_only_the_managed_policy_from_user_file(self, tmp_path): + path = self._seed(tmp_path) + manifest = self._setup(tmp_path) + get_integration("codex").teardown(tmp_path, manifest) + assert self._yaml(path) == {"interface": {"display_name": "Plan"}} + assert self._other_policies(tmp_path) == [] + + def test_user_file_survives_upgrade_option_clearing_and_uninstall(self, tmp_path): + from tests.specify_cli.integrations._helpers import _run_in_project + + path = self._seed(tmp_path) + self._init(tmp_path, "--no-model-invocation") + assert self._manifest(tmp_path).ownership_modes[self.PLAN_REL] == "partial" + + result = _run_in_project(tmp_path, ["integration", "upgrade", "codex", "--force"]) + assert result.exit_code == 0, result.output + assert self._manifest(tmp_path).ownership_modes[self.PLAN_REL] == "partial" + assert self._yaml(path)["policy"] == self.POLICY + + result = _run_in_project( + tmp_path, ["integration", "upgrade", "codex", "--force", "--integration-options", ""] + ) + assert result.exit_code == 0, result.output + assert self._yaml(path) == {"interface": {"display_name": "Plan"}} + assert self._other_policies(tmp_path) == [] + assert not [k for k in self._manifest(tmp_path).files if k.endswith("openai.yaml")] + + result = _run_in_project( + tmp_path, + ["integration", "upgrade", "codex", "--force", + "--integration-options", "--no-model-invocation"], + ) + assert result.exit_code == 0, result.output + assert self._yaml(path)["policy"] == self.POLICY + + result = _run_in_project(tmp_path, ["integration", "uninstall", "codex"]) + assert result.exit_code == 0, result.output + assert self._yaml(path) == {"interface": {"display_name": "Plan"}} + assert self._other_policies(tmp_path) == [] + + def test_reinit_without_option_reconciles_previous_policies(self, tmp_path): + path = self._seed(tmp_path) + self._init(tmp_path, "--no-model-invocation") + assert self._other_policies(tmp_path) + + self._init(tmp_path) + assert self._yaml(path) == {"interface": {"display_name": "Plan"}} + assert self._other_policies(tmp_path) == [] + assert not list((tmp_path / ".agents" / "skills").glob("speckit-specify/agents")) + assert not [k for k in self._manifest(tmp_path).files if k.endswith("openai.yaml")] + + def test_clearing_option_keeps_user_edits_to_a_generated_file(self, tmp_path): + from tests.specify_cli.integrations._helpers import _run_in_project + + self._init(tmp_path, "--no-model-invocation") + path = tmp_path / self.PLAN_REL + assert self._manifest(tmp_path).ownership_modes[self.PLAN_REL] == "whole" + path.write_text( + "interface:\n display_name: Plan\npolicy:\n allow_implicit_invocation: false\n", + encoding="utf-8", + ) + + result = _run_in_project( + tmp_path, ["integration", "upgrade", "codex", "--force", "--integration-options", ""] + ) + assert result.exit_code == 0, result.output + assert self._yaml(path) == {"interface": {"display_name": "Plan"}} + assert self._other_policies(tmp_path) == [] From 06cafd3e32ca579a345500f5f17f049631f3a935 Mon Sep 17 00:00:00 2001 From: Ed Harrod Date: Wed, 28 Oct 2026 10:44:07 +0000 Subject: [PATCH 3/4] fix(codex): harden openai.yaml policy cleanup edge cases Validate existing policy files before the base setup writes any skill, and derive manifest paths from the resolved project root. Treat a generated file whose content still equals the policy (CRLF checkout, added comment) as generated, so cleanup deletes it rather than leaving a stub. Keep tracking a user-edited generated file as partial even when it no longer parses, so upgrade's stale-file pass cannot delete it. Do not report user files whose policy teardown removed as preserved modified files. Document that rewritten files lose comments and formatting. Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015w5WirGUspLsYEpCkaHWxV --- docs/integrations/codex.md | 3 +- .../integrations/codex/__init__.py | 109 +++++++++++------- tests/integrations/test_integration_codex.py | 48 ++++++++ 3 files changed, 118 insertions(+), 42 deletions(-) diff --git a/docs/integrations/codex.md b/docs/integrations/codex.md index dbfa693e65..e5e813375f 100644 --- a/docs/integrations/codex.md +++ b/docs/integrations/codex.md @@ -56,7 +56,8 @@ description in Codex's context. codex`) deletes the `agents/openai.yaml` files Spec Kit generated. An `agents/openai.yaml` that already existed, or that you edited, keeps its other keys: only `policy.allow_implicit_invocation` is set, and later removed again. - One that already sets it to `false` is left untouched. If an existing file + Such a file is rewritten as plain YAML, so its comments and formatting are + not kept. One that already sets it to `false` is left untouched. If an existing file cannot be read, is not valid YAML, or is not a mapping, setup stops with an error naming the file and leaves it unchanged. Skills that commands other than `init`, `install` and `upgrade` register (for example presets that add core diff --git a/src/specify_cli/integrations/codex/__init__.py b/src/specify_cli/integrations/codex/__init__.py index 5a8e4a799c..d55fa845c7 100644 --- a/src/specify_cli/integrations/codex/__init__.py +++ b/src/specify_cli/integrations/codex/__init__.py @@ -52,6 +52,8 @@ class CodexIntegration(SkillsIntegration): events_config_file = ".codex/config.toml" events_format = "toml" + _GENERATED_POLICY: dict[str, Any] = {"policy": {"allow_implicit_invocation": False}} + @staticmethod def _load_openai_yaml(path: Path, rel: str) -> dict[str, Any]: """Parse an existing ``openai.yaml``; raise rather than guess at unusable content.""" @@ -95,11 +97,22 @@ def _prior_manifest(self, project_root: Path) -> IntegrationManifest | None: ) return None - @staticmethod - def _matches_recorded(path: Path, manifest: IntegrationManifest, rel: str) -> bool: + def _is_generated(self, root: Path, rel: str, prior: IntegrationManifest | None) -> bool: + """Whether ``rel`` is a policy-only file an earlier install generated. + + Content equality also counts, so a CRLF checkout or an added comment + does not turn Spec Kit's own file into a user file. + """ + path = root / rel + if prior is None or prior.ownership_modes.get(rel) != "whole": + return False + if path.is_symlink() or not path.is_file(): + return False try: - return hashlib.sha256(path.read_bytes()).hexdigest() == manifest.files.get(rel) - except OSError: + if hashlib.sha256(path.read_bytes()).hexdigest() == prior.files[rel]: + return True + return self._load_openai_yaml(path, rel) == self._GENERATED_POLICY + except (OSError, ValueError): return False def _plan_policy( @@ -117,12 +130,7 @@ def _plan_policy( raise ValueError(f"Cannot update {rel}: not a regular file") else: data = self._load_openai_yaml(path, rel) - generated = ( - tracked - and prior.ownership_modes[rel] == "whole" - and self._matches_recorded(path, prior, rel) - ) - ownership = "whole" if generated else "partial" + ownership = "whole" if self._is_generated(project_root, rel, prior) else "partial" policy = data.get("policy") or {} if ownership == "partial" and not tracked and policy.get("allow_implicit_invocation") is False: # The user already set it; claiming it would let cleanup remove their setting. @@ -132,22 +140,22 @@ def _plan_policy( return rel, yaml.safe_dump(data, sort_keys=False), ownership def _remove_policy(self, project_root: Path, rel: str) -> bool: - """Drop the managed policy from a user-owned file; return whether the file remains.""" + """Drop the managed policy from a user-owned file; return whether it is now policy-free.""" path = project_root / rel try: _validate_record_path(Path(rel), project_root) if not path.is_file(): return False data = self._load_openai_yaml(path, rel) + policy = data.get("policy") + if isinstance(policy, dict) and policy.get("allow_implicit_invocation") is False: + del policy["allow_implicit_invocation"] + if not policy: + del data["policy"] + write_bytes(path, yaml.safe_dump(data, sort_keys=False).encode("utf-8")) except (OSError, ValueError) as exc: logger.warning("Leaving %s unchanged: %s", rel, exc) return False - policy = data.get("policy") - if isinstance(policy, dict) and policy.get("allow_implicit_invocation") is False: - del policy["allow_implicit_invocation"] - if not policy: - del data["policy"] - write_bytes(path, yaml.safe_dump(data, sort_keys=False).encode("utf-8")) return True def _release_prior_policies( @@ -166,14 +174,21 @@ def _release_prior_policies( for rel, recorded_hash in prior.files.items(): if rel in keep or not self._is_policy_rel(rel): continue - path = project_root / rel - if modes[rel] == "whole" and self._matches_recorded(path, prior, rel): + if self._is_generated(project_root, rel, prior): generated._files[rel] = recorded_hash - elif self._remove_policy(project_root, rel) and modes[rel] == "whole": - # A generated file the user edited is theirs now. Track it as - # partial so upgrade's stale-file pass does not delete it. - manifest.record_existing(rel, ownership="partial") - generated.uninstall(project_root, remove_manifest=False) + continue + self._remove_policy(project_root, rel) + path = project_root / rel + if modes[rel] == "whole" and path.is_file() and not path.is_symlink(): + # A generated file the user edited is theirs now, even if it no + # longer parses. Track it as partial so upgrade's stale-file + # pass, which deletes whole files with force, leaves it alone. + try: + manifest.record_existing(rel, ownership="partial") + except (OSError, ValueError) as exc: + logger.warning("Could not keep tracking %s: %s", rel, exc) + # Each entry was checked above, content-equal ones included. + generated.uninstall(project_root, force=True, remove_manifest=False) @staticmethod def _is_core_command_skill_dir(name: str) -> bool: @@ -190,28 +205,35 @@ def setup( parsed_options: dict[str, Any] | None = None, **opts: Any, ) -> list[Path]: - created = super().setup( - project_root, manifest, parsed_options=parsed_options, **opts - ) # Codex has no SKILL.md frontmatter flag; the policy lives in a sibling # ``agents/openai.yaml``. It is written for core command skills only, # since mandatory extension hooks rely on the model invoking them. - # Every file is validated before any is written. + root = project_root.resolve() + enabled = (parsed_options or {}).get("no_model_invocation") is True + prior = self._prior_manifest(root) + if enabled: + # Reject unusable existing policy files before anything is written. + from specify_cli.extensions import CORE_COMMAND_NAMES # lazy: avoids circular import + + skills = PurePosixPath(self.registrar_config["dir"]) + for name in sorted(CORE_COMMAND_NAMES): + rel = (skills / f"speckit-{name}" / "agents" / "openai.yaml").as_posix() + agents_dir = (root / rel).parent + if agents_dir.exists() or agents_dir.is_symlink(): + self._plan_policy(root, rel, prior) + created = super().setup( + project_root, manifest, parsed_options=parsed_options, **opts + ) planned: list[tuple[str, str, str]] = [] - prior = self._prior_manifest(project_root) - if (parsed_options or {}).get("no_model_invocation") is True: + if enabled: for skill_file in [f for f in created if f.name == "SKILL.md"]: if self._is_core_command_skill_dir(skill_file.parent.name): dest = skill_file.parent / "agents" / "openai.yaml" - plan = self._plan_policy( - project_root, dest.relative_to(project_root).as_posix(), prior - ) + plan = self._plan_policy(root, dest.relative_to(root).as_posix(), prior) if plan is not None: planned.append(plan) if prior is not None: - self._release_prior_policies( - project_root, manifest, prior, {rel for rel, _, _ in planned} - ) + self._release_prior_policies(root, manifest, prior, {rel for rel, _, _ in planned}) for rel, content, ownership in planned: created.append(manifest.record_file(rel, content, ownership=ownership)) return created @@ -224,11 +246,16 @@ def teardown( force: bool = False, ) -> tuple[list[Path], list[Path]]: # ``manifest.uninstall`` never touches partial files, so remove the - # managed policy from user-owned ``openai.yaml`` files here. - for rel, mode in manifest.ownership_modes.items(): - if mode != "whole" and self._is_policy_rel(rel): - self._remove_policy(project_root, rel) - return super().teardown(project_root, manifest, force=force) + # managed policy from user-owned ``openai.yaml`` files here, and do not + # report the ones cleaned as preserved modified files. + root = project_root.resolve() + cleaned = { + root / rel + for rel, mode in manifest.ownership_modes.items() + if mode != "whole" and self._is_policy_rel(rel) and self._remove_policy(root, rel) + } + removed, skipped = super().teardown(project_root, manifest, force=force) + return removed, [path for path in skipped if path not in cleaned] def build_exec_args( self, diff --git a/tests/integrations/test_integration_codex.py b/tests/integrations/test_integration_codex.py index 5313c86ffe..24ce7599ae 100644 --- a/tests/integrations/test_integration_codex.py +++ b/tests/integrations/test_integration_codex.py @@ -396,6 +396,7 @@ def test_unusable_existing_file_aborts_without_overwriting(self, tmp_path, conte self._setup(tmp_path) assert path.read_bytes() == content assert self._other_policies(tmp_path) == [] + assert not list((tmp_path / ".agents" / "skills").glob("*/SKILL.md")) def test_unreadable_existing_file_aborts_without_overwriting(self, tmp_path, monkeypatch): path = self._seed(tmp_path) @@ -519,3 +520,50 @@ def test_clearing_option_keeps_user_edits_to_a_generated_file(self, tmp_path): assert result.exit_code == 0, result.output assert self._yaml(path) == {"interface": {"display_name": "Plan"}} assert self._other_policies(tmp_path) == [] + + def test_teardown_does_not_report_cleaned_user_file_as_preserved(self, tmp_path): + path = self._seed(tmp_path) + manifest = self._setup(tmp_path) + _, skipped = get_integration("codex").teardown(tmp_path, manifest) + assert path.resolve() not in [p.resolve() for p in skipped] + + def test_setup_accepts_unresolved_project_root(self, tmp_path): + if not hasattr(os, "symlink"): + pytest.skip("symlinks are unavailable") + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + manifest = self._setup(link) + assert self._yaml(real / self.PLAN_REL) == {"policy": self.POLICY} + assert self.PLAN_REL in manifest.files + + def test_clearing_option_keeps_unparseable_edit_of_generated_file(self, tmp_path): + from tests.specify_cli.integrations._helpers import _run_in_project + + self._init(tmp_path, "--no-model-invocation") + path = tmp_path / self.PLAN_REL + path.write_bytes(b"interface: [half-edited\n") + + result = _run_in_project( + tmp_path, ["integration", "upgrade", "codex", "--force", "--integration-options", ""] + ) + assert result.exit_code == 0, result.output + assert path.read_bytes() == b"interface: [half-edited\n" + assert self._other_policies(tmp_path) == [] + + def test_clearing_option_deletes_generated_file_after_cosmetic_change(self, tmp_path): + from tests.specify_cli.integrations._helpers import _run_in_project + + self._init(tmp_path, "--no-model-invocation") + path = tmp_path / self.PLAN_REL + path.write_bytes(path.read_bytes().replace(b"\n", b"\r\n") + b"# note\r\n") + + result = _run_in_project(tmp_path, ["integration", "upgrade", "codex", "--force"]) + assert result.exit_code == 0, result.output + assert self._manifest(tmp_path).ownership_modes[self.PLAN_REL] == "whole" + path.write_bytes(path.read_bytes().replace(b"\n", b"\r\n") + b"# note\r\n") + + self._init(tmp_path) + assert not path.exists() + assert self._other_policies(tmp_path) == [] From 4788fbd718665aa5546bdbd146a922090eaa2bfb Mon Sep 17 00:00:00 2001 From: Ed Harrod Date: Wed, 28 Oct 2026 10:57:31 +0000 Subject: [PATCH 4/4] fix(codex): undo the policy in edited generated files on uninstall Teardown now treats generated openai.yaml files like the option-clearing path does: a content-equal file (CRLF, added comment) is deleted, and an edited one keeps its keys and loses only the policy unless --force is given. A user file whose policy cannot be removed stays tracked so a later run can retry, and stale policies are released only after the new ones are written. Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015w5WirGUspLsYEpCkaHWxV --- .../integrations/codex/__init__.py | 32 ++++++++++++------- tests/integrations/test_integration_codex.py | 27 ++++++++++++++++ 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/integrations/codex/__init__.py b/src/specify_cli/integrations/codex/__init__.py index d55fa845c7..9b55547c09 100644 --- a/src/specify_cli/integrations/codex/__init__.py +++ b/src/specify_cli/integrations/codex/__init__.py @@ -177,12 +177,14 @@ def _release_prior_policies( if self._is_generated(project_root, rel, prior): generated._files[rel] = recorded_hash continue - self._remove_policy(project_root, rel) + cleaned = self._remove_policy(project_root, rel) path = project_root / rel - if modes[rel] == "whole" and path.is_file() and not path.is_symlink(): + if (modes[rel] == "whole" or not cleaned) and path.is_file() and not path.is_symlink(): # A generated file the user edited is theirs now, even if it no # longer parses. Track it as partial so upgrade's stale-file - # pass, which deletes whole files with force, leaves it alone. + # pass, which deletes whole files with force, leaves it alone; + # a file whose policy could not be removed stays tracked so a + # later run can retry. try: manifest.record_existing(rel, ownership="partial") except (OSError, ValueError) as exc: @@ -232,10 +234,10 @@ def setup( plan = self._plan_policy(root, dest.relative_to(root).as_posix(), prior) if plan is not None: planned.append(plan) - if prior is not None: - self._release_prior_policies(root, manifest, prior, {rel for rel, _, _ in planned}) for rel, content, ownership in planned: created.append(manifest.record_file(rel, content, ownership=ownership)) + if prior is not None: + self._release_prior_policies(root, manifest, prior, {rel for rel, _, _ in planned}) return created def teardown( @@ -247,13 +249,21 @@ def teardown( ) -> tuple[list[Path], list[Path]]: # ``manifest.uninstall`` never touches partial files, so remove the # managed policy from user-owned ``openai.yaml`` files here, and do not - # report the ones cleaned as preserved modified files. + # report the ones cleaned as preserved modified files. Generated files + # are left to ``uninstall``: content-equal ones are re-hashed so it + # deletes them, and edited ones lose only the policy unless forced. root = project_root.resolve() - cleaned = { - root / rel - for rel, mode in manifest.ownership_modes.items() - if mode != "whole" and self._is_policy_rel(rel) and self._remove_policy(root, rel) - } + cleaned: set[Path] = set() + for rel, mode in manifest.ownership_modes.items(): + if not self._is_policy_rel(rel): + continue + if mode != "whole": + if self._remove_policy(root, rel): + cleaned.add(root / rel) + elif self._is_generated(root, rel, manifest): + manifest.record_existing(rel) + elif not force: + self._remove_policy(root, rel) removed, skipped = super().teardown(project_root, manifest, force=force) return removed, [path for path in skipped if path not in cleaned] diff --git a/tests/integrations/test_integration_codex.py b/tests/integrations/test_integration_codex.py index 24ce7599ae..5da6ed8cb1 100644 --- a/tests/integrations/test_integration_codex.py +++ b/tests/integrations/test_integration_codex.py @@ -567,3 +567,30 @@ def test_clearing_option_deletes_generated_file_after_cosmetic_change(self, tmp_ self._init(tmp_path) assert not path.exists() assert self._other_policies(tmp_path) == [] + + def test_teardown_strips_policy_from_edited_generated_file(self, tmp_path): + manifest = self._setup(tmp_path) + path = tmp_path / self.PLAN_REL + path.write_text( + "interface:\n display_name: Plan\npolicy:\n allow_implicit_invocation: false\n", + encoding="utf-8", + ) + get_integration("codex").teardown(tmp_path, manifest) + assert self._yaml(path) == {"interface": {"display_name": "Plan"}} + assert self._other_policies(tmp_path) == [] + + def test_teardown_deletes_generated_file_after_cosmetic_change(self, tmp_path): + manifest = self._setup(tmp_path) + path = tmp_path / self.PLAN_REL + path.write_bytes(path.read_bytes().replace(b"\n", b"\r\n") + b"# note\r\n") + get_integration("codex").teardown(tmp_path, manifest) + assert not path.exists() + + def test_user_file_whose_policy_cannot_be_removed_stays_tracked(self, tmp_path): + path = self._seed(tmp_path) + self._setup(tmp_path).save() + broken = path.read_bytes() + b"interface: [broken\n" + path.write_bytes(broken) + manifest = self._setup(tmp_path, enabled=False) + assert path.read_bytes() == broken + assert manifest.ownership_modes[self.PLAN_REL] == "partial"