diff --git a/.claude/notes/agents.md b/.claude/notes/agents.md index b6d1e1474..365fc01db 100644 --- a/.claude/notes/agents.md +++ b/.claude/notes/agents.md @@ -717,8 +717,15 @@ being hardcoded, so a plugin that relocates its skills keeps working. skips external *plugins*, not configured skill paths. An inherited value is appended to rather than clobbered, since the host may legitimately configure OpenCode the same way. - **Pi** passes each as `--skill `. -- **Codex** symlinks (or copies, on Windows) each skill dir into `.agents/skills/`, which - the CLI auto-discovers from the working directory upward. +- **Codex** copies each skill dir into `.agents/skills/`, which the CLI auto-discovers from + the working directory upward. Copies, not symlinks, for two reasons that compound. Codex + resolves a symlinked skill to its source, finds the plugin manifest there and lists the + skill as `:`; models sometimes turn that into the nonexistent path + `.agents/skills///SKILL.md`. The search that follows (`rg --files`, + `find`) skips symlinks and returns nothing, so the agent works without the skill. The + copies are made before each turn and removed after it, the same concern that kept + OpenCode out of the sandbox: criteria and run artifacts must see only what the agent + left. Skill dirs the workspace already has are never touched. - **Antigravity** takes search paths natively via `skills_paths` — but those only drive DISCOVERY. The file-tool allowlist is `workspaces` alone, so the skill roots must appear there too, or the agent discovers a skill and every read of its `SKILL.md` is denied as diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index d3caa64d9..e8f0c823e 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -1345,7 +1345,7 @@ Like `skill_triggered`, this criterion emits a `ClassificationCriterionResult`, ### `skill_triggered` -Binary classifier: **did the agent engage the target skill during the run?** Agent-agnostic — scans the run's `turn_records` for either signal: Claude's explicit `Skill` tool call whose `skill` parameter matches `skill_name` (namespace prefixes like `plugin:skill` are stripped, so `skill_name: uipath-agents` matches `Skill(skill="uipath-coded-agents:uipath-agents")`), or — for an agent with no `Skill` tool, e.g. Codex — a command that reads the skill's files off disk (a parameter contains `skills//`, matching both the repo path and the `.agents/skills/` symlink). +Binary classifier: **did the agent engage the target skill during the run?** Agent-agnostic — scans the run's `turn_records` for either signal: Claude's explicit `Skill` tool call whose `skill` parameter matches `skill_name` (namespace prefixes like `plugin:skill` are stripped, so `skill_name: uipath-agents` matches `Skill(skill="uipath-coded-agents:uipath-agents")`), or — for an agent with no `Skill` tool, e.g. Codex — a command that reads the skill's files off disk (a parameter contains `skills//`, matching both the repo path and the `.agents/skills/` copy). Observed label is `"yes"` when either signal is found, else `"no"`. Expected label is `"yes"` iff `expected_skill == skill_name`. **Binary scoring:** `1.0` when observed matches expected, else `0.0`. diff --git a/docs/agents/ANTIGRAVITY.md b/docs/agents/ANTIGRAVITY.md index a44f43783..ffb9a7943 100644 --- a/docs/agents/ANTIGRAVITY.md +++ b/docs/agents/ANTIGRAVITY.md @@ -127,7 +127,7 @@ Antigravity supports [Agent Skills](https://agentskills.io/specification) 2. the runtime plugin directory passed to `start()`. For each source, the harness is handed whichever of `/skills` or `` -directly contains subdirectories with a `SKILL.md`. Unlike Codex — which symlinks +directly contains subdirectories with a `SKILL.md`. Unlike Codex — which copies skills into `.agents/skills/` — Antigravity is given the search-path roots directly via the SDK's `skills_paths`, and those roots are also added to the harness `workspaces` allowlist (otherwise the agent would find a skill but be denied the diff --git a/docs/agents/CODEX.md b/docs/agents/CODEX.md index 12b8e33e1..4e86e7011 100644 --- a/docs/agents/CODEX.md +++ b/docs/agents/CODEX.md @@ -115,7 +115,7 @@ CodexAgent supports SKILL.md files following the [Agent Skills open standard](ht 1. **config.plugins** - Local plugins with `type: local` and `path` pointing to a skills directory 2. **plugin_tools_dir** parameter - Runtime plugin directory passed to `start()` -Skills are symlinked (or copied) to `.agents/skills/` where the Codex CLI auto-discovers them. Environment variables in plugin paths (`$VAR`, `${VAR}`) are expanded at runtime. +Skills are copied to `.agents/skills/` for each turn, where the Codex CLI auto-discovers them, and removed when the turn ends. Environment variables in plugin paths (`$VAR`, `${VAR}`) are expanded at runtime. Example with environment variable: ```yaml @@ -206,8 +206,9 @@ The agent sets up SKILL.md files (Agent Skills open standard) in `.agents/skills 1. Scans `config.plugins` for local plugins with `path` field 2. Checks `plugin_tools_dir` parameter passed to `start()` 3. Expands environment variables in paths (`$PLUGIN_PATH`, `${PLUGIN_PATH}`) -4. Symlinks skill directories (falls back to copying if symlink fails) +4. Copies each skill directory into `.agents/skills/` before every turn (real files, not symlinks), skipping any skill dir the workspace already has 5. Codex CLI auto-discovers skills in `.agents/skills/` +6. Removes the copies when the turn ends, so criteria and run artifacts never see them ### Async Integration diff --git a/docs/agents/HARNESS_PARITY.md b/docs/agents/HARNESS_PARITY.md index d63cea6a0..dc751c867 100644 --- a/docs/agents/HARNESS_PARITY.md +++ b/docs/agents/HARNESS_PARITY.md @@ -684,7 +684,7 @@ This field breaks it silently. claude-code hands the value to the SDK as a *plugin directory*, and a plugin's skills live at `/skills//SKILL.md`. Point it at the directory that directly parents the skill directories and no skill loads. Codex -(`codex_agent._setup_skills`) and Antigravity (`antigravity_agent._resolve_skills_paths`) +(`codex_agent._collect_skills`) and Antigravity (`antigravity_agent._resolve_skills_paths`) both scan **both** layouts and take whichever actually holds a `/SKILL.md`. † Pi uses the shared `_plugin_skill_dirs` resolver, whose bare-dir fallback resolves a diff --git a/src/coder_eval/agents/codex_agent.py b/src/coder_eval/agents/codex_agent.py index 2430eda3b..e37bc73a7 100644 --- a/src/coder_eval/agents/codex_agent.py +++ b/src/coder_eval/agents/codex_agent.py @@ -854,6 +854,9 @@ def __init__( self.working_directory: Path | None = None self._env_path_prepend: list[str] = [] self._login_shell_home: Path | None = None + self._skill_dirs: list[Path] = [] + self._skill_copies: list[Path] = [] + self._skill_parent_dirs: list[Path] = [] # _state / _iteration / _iteration_was_incremented / pending_turn lifecycle # bookkeeping lives on the Agent base class (shared defaults + helpers). self._log = PrefixedAdapter(logger, {"prefix": instance_name}) @@ -909,8 +912,7 @@ async def start( "CodexAgent: login_api_key failed — agent will fall back to env-based auth: %s", exc ) - # Set up skills from plugin_tools_dir or plugins config - self._setup_skills(plugin_tools_dir) + self._collect_skills(plugin_tools_dir) # Log permission and tool configuration self._log_config_enforcement() @@ -954,6 +956,27 @@ async def communicate( if not self.working_directory or not self.codex_client: raise RuntimeError("Agent not started. Call start() first.") + self._install_skills() + try: + return await self._communicate_turn( + user_input, + stream_callback=stream_callback, + timeout=timeout, + max_turns=max_turns, + should_stop=should_stop, + ) + finally: + self._remove_skills() + + async def _communicate_turn( + self, + user_input: str, + *, + stream_callback: StreamCallback | None, + timeout: float | None, + max_turns: int | None, + should_stop: Callable[[], bool] | None, + ) -> TurnRecord: assert self.config.type is not None, "CodexAgent requires AgentConfig.type to be set before communicate()" # Reset the pending slot + bump the iteration counter (shared lifecycle). @@ -1092,6 +1115,7 @@ async def stop(self) -> None: self._thread_usage_baseline = _ThreadTotals() self._active_turn_handle = None self._cleanup_login_shell_home() + self._remove_skills() self._mark_stopped() async def kill(self) -> None: @@ -1108,6 +1132,7 @@ def kill_sync(self) -> None: self._interrupt_active_turn() self._close_client() self._cleanup_login_shell_home() + self._remove_skills() def _interrupt_active_turn(self) -> None: """Interrupt the in-flight Codex turn, if any (best-effort, idempotent).""" @@ -1147,17 +1172,16 @@ def get_environment_info(self) -> dict[str, Any]: "codex_model_is_deployment": True, } - def _setup_skills(self, plugin_tools_dir: str | None) -> None: - """Set up .agents/skills directory from plugins or plugin_tools_dir. + def _collect_skills(self, plugin_tools_dir: str | None) -> None: + """Resolve the skill dirs that :meth:`_install_skills` copies in for each turn. - Codex auto-discovers skills in ``.agents/skills/``, scanned from the working - directory up to the repo root, so each source's skill dirs are symlinked - (or copied, on Windows) into it. + A source may hold skill dirs directly or be a plugin root whose skills live + one level deeper; both layouts are scanned, and the first source to provide + a skill name wins. Rationale: .claude/notes/agents.md § Skills, per harness """ - if not self.working_directory: - return + self._skill_dirs = [] skills_sources: list[Path] = [] @@ -1179,7 +1203,7 @@ def _setup_skills(self, plugin_tools_dir: str | None) -> None: hint = "env var likely unset" if "$" in expanded_path else "path does not exist" self._log.warning( f"Plugin skills path did not resolve: {path_str!r} " - + f"→ {expanded_path!r} ({hint}); no skills linked from it" + + f"→ {expanded_path!r} ({hint}); no skills taken from it" ) # Also check plugin_tools_dir parameter @@ -1192,53 +1216,75 @@ def _setup_skills(self, plugin_tools_dir: str | None) -> None: if not skills_sources: return - # Create .agents/skills directory (Codex auto-discovery location) - agents_skills_dir = self.working_directory / ".agents" / "skills" try: - agents_skills_dir.mkdir(parents=True, exist_ok=True) - - # A source may hold skill dirs directly or be a plugin root whose - # skills live one level deeper. Scan both layouts. for skills_source in skills_sources: scan_dirs = [skills_source] nested = skills_source / "skills" if nested.is_dir(): scan_dirs.append(nested) - for scan_dir in scan_dirs: for skill_dir in scan_dir.iterdir(): if not (skill_dir.is_dir() and (skill_dir / "SKILL.md").exists()): continue - target = agents_skills_dir / skill_dir.name - if target.exists(): - # Skip if already exists (first source wins) - continue + if all(d.name != skill_dir.name for d in self._skill_dirs): + self._skill_dirs.append(skill_dir) + except OSError as e: + self._log.warning(f"Failed to scan skills: {e}") - try: - # Try to create a symlink for efficiency - target.symlink_to(skill_dir) - self._log.debug(f"Linked skill: {skill_dir.name}") - except (OSError, NotImplementedError): - # Fall back to copying if symlink fails (Windows compatibility) - shutil.copytree(skill_dir, target, dirs_exist_ok=True) - self._log.debug(f"Copied skill: {skill_dir.name}") - - linked = list(agents_skills_dir.iterdir()) - if linked: - self._log.debug(f"Linked {len(linked)} skill(s) into {agents_skills_dir}") - else: - # Sources existed but held no SKILL.md, so codex runs with no - # skill context at all. - self._log.warning( - f"0 skills linked into {agents_skills_dir} despite " - + f"{len(skills_sources)} plugin source(s): " - + f"{[str(s) for s in skills_sources]}; " - + "check the plugin path points at a skills repo root" - ) + if self._skill_dirs: + self._log.debug(f"Found {len(self._skill_dirs)} skill(s): {[d.name for d in self._skill_dirs]}") + else: + # Sources existed but held no SKILL.md, so codex runs with no + # skill context at all. + self._log.warning( + f"0 skills found in {len(skills_sources)} plugin source(s): " + + f"{[str(s) for s in skills_sources]}; " + + "check the plugin path points at a skills repo root" + ) + def _install_skills(self) -> None: + """Copy the collected skills into ``.agents/skills/``, where Codex auto-discovers them. + + Real copies, not symlinks. Codex resolves a symlinked skill to its source, + finds the plugin manifest there and lists it as ``:``, which + models turn into a nonexistent ``//`` path; ``rg --files`` and + ``find`` then skip the symlinks, so the search that follows finds nothing. A + skill dir already in the workspace is left alone and never removed. + + Rationale: .claude/notes/agents.md § Skills, per harness + """ + if not (self.working_directory and self._skill_dirs): + return + agents_skills_dir = self.working_directory / ".agents" / "skills" + try: + for parent in (agents_skills_dir.parent, agents_skills_dir): + if not parent.exists(): + parent.mkdir() + self._skill_parent_dirs.append(parent) + for skill_dir in self._skill_dirs: + target = agents_skills_dir / skill_dir.name + if target.exists() or target.is_symlink(): + continue + self._skill_copies.append(target) + shutil.copytree(skill_dir, target, symlinks=True) + self._log.debug(f"Copied {len(self._skill_copies)} skill(s) into {agents_skills_dir}") except Exception as e: self._log.warning(f"Failed to set up skills: {e}") + def _remove_skills(self) -> None: + """Remove what :meth:`_install_skills` created (best-effort, idempotent). + + Runs at the end of every turn, so criteria and run artifacts see the workspace + the agent left, not the skill copies. + """ + copies, self._skill_copies = self._skill_copies, [] + parents, self._skill_parent_dirs = self._skill_parent_dirs, [] + for copy in copies: + shutil.rmtree(copy, ignore_errors=True) + for parent in reversed(parents): + with contextlib.suppress(OSError): + parent.rmdir() + @staticmethod def _resolve_base_url() -> str | None: """Custom Codex endpoint base URL from CODEX_BASE_URL, or None.""" diff --git a/tasks/agents/codex_skills_test.yaml b/tasks/agents/codex_skills_test.yaml index acddfc874..00a02f575 100644 --- a/tasks/agents/codex_skills_test.yaml +++ b/tasks/agents/codex_skills_test.yaml @@ -1,5 +1,5 @@ task_id: codex_skills_test -description: Test that CodexAgent properly sets up and discovers SKILL.md files +description: Test that CodexAgent installs SKILL.md files for the turn and removes them before grading tags: - codex @@ -15,16 +15,16 @@ agent: path: "$PLUGIN_PATH" initial_prompt: | - List all available skills in your environment by checking the .agents/skills directory. - Count how many SKILL.md files are available. + Run `find .agents/skills -name SKILL.md | wc -l` and write the number it prints, and + nothing else, to skill_count.txt. success_criteria: - type: run_command - description: .agents/skills directory should exist - command: test -d .agents/skills + description: find should see at least one SKILL.md under .agents/skills + command: grep -Eq '^[[:space:]]*[1-9][0-9]*[[:space:]]*$' skill_count.txt expected_exit_code: 0 - type: run_command - description: Should have at least one SKILL.md file - command: find .agents/skills -name "SKILL.md" | wc -l | grep -v "^0$" + description: The skill copies are gone by grading time + command: test ! -e .agents expected_exit_code: 0 diff --git a/tests/test_codex_agent_unit.py b/tests/test_codex_agent_unit.py index 3d144c655..982eecfec 100644 --- a/tests/test_codex_agent_unit.py +++ b/tests/test_codex_agent_unit.py @@ -14,8 +14,10 @@ from __future__ import annotations +import os import time from datetime import datetime +from pathlib import Path from types import SimpleNamespace import pytest @@ -272,3 +274,77 @@ def test_generation_completed_at_is_left_unset(self, factory, root_type): # tool_use block; deriving it from the flush time would be a guess. tel = self._build(factory(), root_type, started_ms=_EPOCH_MS, completed_ms=_EPOCH_MS + 1) assert tel.generation_completed_at is None + + +def _skills_repo(root: Path, *names: str) -> Path: + for name in names: + (root / "skills" / name / "references").mkdir(parents=True) + (root / "skills" / name / "SKILL.md").write_text("---\nname: x\n---\n", encoding="utf-8") + (root / "skills" / name / "references" / "guide.md").write_text("ref\n", encoding="utf-8") + return root + + +class TestSkillCopies: + """Skills reach ``.agents/skills/`` as real files for each turn, and leave with it.""" + + @staticmethod + def _agent(tmp_path: Path, *names: str) -> CodexAgent: + plugin = _skills_repo(tmp_path / "plugin", *names) + agent = CodexAgent( + parse_agent_config(type=AgentKind.CODEX, model="gpt-5.5", plugins=[{"type": "local", "path": str(plugin)}]) + ) + agent.working_directory = tmp_path / "work" + agent.working_directory.mkdir() + agent._collect_skills(None) + return agent + + def test_a_walk_that_skips_symlinks_still_lists_the_skill(self, tmp_path): + # os.walk without followlinks sees what `rg --files` and `find` see. + agent = self._agent(tmp_path, "uipath-planner") + agent._install_skills() + skills_root = agent.working_directory / ".agents" / "skills" + found = sorted( + (Path(d) / f).relative_to(skills_root).as_posix() for d, _, files in os.walk(skills_root) for f in files + ) + assert found == ["uipath-planner/SKILL.md", "uipath-planner/references/guide.md"] + + def test_remove_restores_the_workspace(self, tmp_path): + agent = self._agent(tmp_path, "uipath-planner") + agent._install_skills() + agent._remove_skills() + assert list(agent.working_directory.iterdir()) == [] + + def test_workspace_skill_dirs_and_agent_files_survive(self, tmp_path): + agent = self._agent(tmp_path, "uipath-planner", "uipath-rpa") + own = agent.working_directory / ".agents" / "skills" / "uipath-rpa" + own.mkdir(parents=True) + (own / "SKILL.md").write_text("task-provided\n", encoding="utf-8") + agent._install_skills() + (agent.working_directory / ".agents" / "notes.md").write_text("agent output\n", encoding="utf-8") + agent._remove_skills() + assert (own / "SKILL.md").read_text(encoding="utf-8") == "task-provided\n" + assert (agent.working_directory / ".agents" / "notes.md").is_file() + assert not (agent.working_directory / ".agents" / "skills" / "uipath-planner").exists() + + @pytest.mark.parametrize("turn_raises", [False, True]) + async def test_skills_exist_only_during_each_turn(self, tmp_path, monkeypatch, turn_raises): + agent = self._agent(tmp_path, "uipath-planner") + agent.codex_client = object() + skill_md = agent.working_directory / ".agents" / "skills" / "uipath-planner" / "SKILL.md" + seen: list[bool] = [] + + async def fake_turn(user_input, **_): + seen.append(skill_md.is_file()) + if turn_raises: + raise RuntimeError("turn failed") + return "record" + + monkeypatch.setattr(agent, "_communicate_turn", fake_turn) + for _ in range(2): + if turn_raises: + with pytest.raises(RuntimeError): + await agent.communicate("go") + else: + assert await agent.communicate("go") == "record" + assert not (agent.working_directory / ".agents").exists() + assert seen == [True, True] diff --git a/tests/test_docker_runner_mounts.py b/tests/test_docker_runner_mounts.py index 234d2ae32..2c47bd11a 100644 --- a/tests/test_docker_runner_mounts.py +++ b/tests/test_docker_runner_mounts.py @@ -801,7 +801,7 @@ def test_plugin_root_masks_non_skill_children(self, tmp_path: Path): assert f"{CONTAINER_PLUGINS_DIR}/0/.claude-plugin" not in tmpfs def test_tmpfs_targets_are_under_mounted_container_root(self, tmp_path: Path): - # Codex symlinks / Antigravity search paths dereference the container path + # Codex skill copies / Antigravity search paths read from the container path # the staged task.yaml names, so the mask must sit under that path. root = self._plugin_root(tmp_path / "plugin") (root / "skills" / "demo").mkdir(parents=True) diff --git a/tests/test_plugin_processing.py b/tests/test_plugin_processing.py index a6468b693..4719d39ba 100644 --- a/tests/test_plugin_processing.py +++ b/tests/test_plugin_processing.py @@ -113,9 +113,9 @@ class TestPluginRootWarning: scores 0 and the suite reports recall 0.0, indistinguishable from a skill that never triggers. That shipped in six documentation surfaces before anyone noticed. - Codex (`codex_agent._setup_skills`) and Antigravity + Codex (`codex_agent._collect_skills`) and Antigravity (`antigravity_agent._resolve_skills_paths`) accept BOTH depths and already log - when they link zero skills. claude-code — the one harness where the wrong depth is + when they find zero skills. claude-code — the one harness where the wrong depth is fatal — was the only one that stayed silent, and a repo-scoped lint rule cannot reach the user repos where `/coder-eval:check-skill` writes these suites. """ diff --git a/tests/test_skill_triggered.py b/tests/test_skill_triggered.py index 03a335fd6..cf9b2bd64 100644 --- a/tests/test_skill_triggered.py +++ b/tests/test_skill_triggered.py @@ -325,7 +325,7 @@ class TestSkillTriggeredCodex: The detectable signal is a command whose recorded text references the skill's directory, ``skills//`` (present in both the repo path and the - ``.agents/skills/`` symlink). + ``.agents/skills/`` copy). """ def _sed(self, path: str) -> CommandTelemetry: @@ -347,7 +347,7 @@ def test_codex_reads_skill_reference_repo_path_tp(self) -> None: ) assert result.score == 1.0 and result.observed_label == "yes" - def test_codex_reads_skill_md_agents_symlink_tp(self) -> None: + def test_codex_reads_skill_md_agents_copy_tp(self) -> None: result = _check( expected_skill="uipath-agents", skill_name="uipath-agents",