fix(model): resolve CLI exec paths for Windows .cmd shims

skillopt/model/backend_config.py

@@ -3,6 +3,7 @@
 
 import json
 import os
+import shutil
 import warnings
 from collections.abc import Mapping
 from typing import Any
@@ -34,10 +35,23 @@ def _coerce_bool_setting(value: Any, *, name: str) -> bool:
     )
 
 
+def _resolve_cli_path(value: str) -> str:
+    """Resolve a CLI name/executable via PATH + PATHEXT.
+
+    On Windows these npm CLIs install as ``.cmd`` shims, and CreateProcess does
+    not search PATHEXT for a bare name (so a bare ``codex`` spawn raises
+    WinError 2). ``shutil.which`` finds the real executable; fall back to the
+    given value so a configured path still passes through unchanged when it
+    cannot be resolved (e.g. a name that is not on this PATH).
+    """
+    resolved = shutil.which(value)
+    return resolved or value
+
+
 OPTIMIZER_BACKEND = normalize_backend_name(os.environ.get("OPTIMIZER_BACKEND", "openai_chat"))
 TARGET_BACKEND = normalize_backend_name(os.environ.get("TARGET_BACKEND", "openai_chat"))
 
-CODEX_EXEC_PATH = os.environ.get("CODEX_EXEC_PATH") or os.environ.get("CODEX_CLI_BIN") or os.environ.get("CODEX_PATH") or "codex"
+CODEX_EXEC_PATH = _resolve_cli_path(os.environ.get("CODEX_EXEC_PATH") or os.environ.get("CODEX_CLI_BIN") or os.environ.get("CODEX_PATH") or "codex")
 CODEX_EXEC_SANDBOX = os.environ.get("CODEX_EXEC_SANDBOX") or os.environ.get("CODEX_SANDBOX_MODE") or os.environ.get("CODEX_SANDBOX") or "workspace-write"
 CODEX_EXEC_PROFILE = os.environ.get("CODEX_EXEC_PROFILE", "")
 _CODEX_EXEC_FULL_AUTO_ENV = os.environ.get("CODEX_EXEC_FULL_AUTO")
@@ -49,13 +63,13 @@ def _coerce_bool_setting(value: Any, *, name: str) -> bool:
 CODEX_EXEC_NETWORK_ACCESS = _parse_bool(os.environ.get("CODEX_EXEC_NETWORK_ACCESS"), False)
 CODEX_EXEC_WEB_SEARCH = _parse_bool(os.environ.get("CODEX_EXEC_WEB_SEARCH"), False)
 CODEX_EXEC_APPROVAL_POLICY = os.environ.get("CODEX_EXEC_APPROVAL_POLICY", "never")
-CLAUDE_CODE_EXEC_PATH = os.environ.get("CLAUDE_CODE_EXEC_PATH", "claude")
+CLAUDE_CODE_EXEC_PATH = _resolve_cli_path(os.environ.get("CLAUDE_CODE_EXEC_PATH", "claude"))
 CLAUDE_CODE_EXEC_PROFILE = os.environ.get("CLAUDE_CODE_EXEC_PROFILE", "")
 CLAUDE_CODE_EXEC_USE_SDK = os.environ.get("CLAUDE_CODE_EXEC_USE_SDK", "auto")
 CLAUDE_CODE_EXEC_EFFORT = os.environ.get("CLAUDE_CODE_EXEC_EFFORT", "medium")
-CURSOR_EXEC_PATH = os.environ.get("CURSOR_EXEC_PATH", "cursor-agent")
+CURSOR_EXEC_PATH = _resolve_cli_path(os.environ.get("CURSOR_EXEC_PATH", "cursor-agent"))
 CURSOR_EXEC_SANDBOX = os.environ.get("CURSOR_EXEC_SANDBOX", "enabled")
-COPILOT_EXEC_PATH = os.environ.get("COPILOT_EXEC_PATH", "copilot")
+COPILOT_EXEC_PATH = _resolve_cli_path(os.environ.get("COPILOT_EXEC_PATH", "copilot"))
 COPILOT_EXEC_HOME = os.environ.get("COPILOT_EXEC_HOME", "")
 COPILOT_EXEC_ALLOW_ALL_TOOLS = (
     "1" if _parse_bool(os.environ.get("COPILOT_EXEC_ALLOW_ALL_TOOLS"), False) else "0"
@@ -222,7 +236,7 @@ def configure_codex_exec(
         else _coerce_bool_setting(web_search, name="codex_exec_web_search")
     )
     if path is not None:
-        CODEX_EXEC_PATH = str(path).strip() or "codex"
+        CODEX_EXEC_PATH = _resolve_cli_path(str(path).strip() or "codex")
         os.environ["CODEX_EXEC_PATH"] = CODEX_EXEC_PATH
         os.environ["CODEX_CLI_BIN"] = CODEX_EXEC_PATH
     if sandbox is not None:
@@ -361,7 +375,7 @@ def configure_claude_code_exec(
 ) -> None:
     global CLAUDE_CODE_EXEC_PATH, CLAUDE_CODE_EXEC_PROFILE, CLAUDE_CODE_EXEC_USE_SDK, CLAUDE_CODE_EXEC_EFFORT, CLAUDE_CODE_EXEC_MAX_THINKING_TOKENS
     if path is not None:
-        CLAUDE_CODE_EXEC_PATH = str(path).strip() or "claude"
+        CLAUDE_CODE_EXEC_PATH = _resolve_cli_path(str(path).strip() or "claude")
         os.environ["CLAUDE_CODE_EXEC_PATH"] = CLAUDE_CODE_EXEC_PATH
     if profile is not None:
         CLAUDE_CODE_EXEC_PROFILE = str(profile).strip()
@@ -398,7 +412,7 @@ def configure_cursor_exec(
 ) -> None:
     global CURSOR_EXEC_PATH, CURSOR_EXEC_SANDBOX
     if path is not None:
-        CURSOR_EXEC_PATH = str(path).strip() or "cursor-agent"
+        CURSOR_EXEC_PATH = _resolve_cli_path(str(path).strip() or "cursor-agent")
         os.environ["CURSOR_EXEC_PATH"] = CURSOR_EXEC_PATH
     if sandbox is not None:
         normalized_sandbox = str(sandbox).strip().lower() or "enabled"
@@ -433,7 +447,7 @@ def configure_copilot_exec(
     """
     global COPILOT_EXEC_PATH, COPILOT_EXEC_HOME, COPILOT_EXEC_ALLOW_ALL_TOOLS
     if path is not None:
-        COPILOT_EXEC_PATH = str(path).strip() or "copilot"
+        COPILOT_EXEC_PATH = _resolve_cli_path(str(path).strip() or "copilot")
         os.environ["COPILOT_EXEC_PATH"] = COPILOT_EXEC_PATH
     if home is not None:
         COPILOT_EXEC_HOME = str(home).strip()
@@ -478,7 +492,7 @@ def configure_copilot_chat(
     global COPILOT_EXEC_PATH, COPILOT_EXEC_HOME
     global COPILOT_CHAT_OPTIMIZER_MODEL, COPILOT_CHAT_TARGET_MODEL, COPILOT_CHAT_TIMEOUT
     if path is not None:
-        COPILOT_EXEC_PATH = str(path).strip() or "copilot"
+        COPILOT_EXEC_PATH = _resolve_cli_path(str(path).strip() or "copilot")
         os.environ["COPILOT_EXEC_PATH"] = COPILOT_EXEC_PATH
     if home is not None:
         COPILOT_EXEC_HOME = str(home).strip()

skillopt/model/codex_harness.py

@@ -2,6 +2,7 @@
 from __future__ import annotations
 
 import asyncio
+import errno
 import json
 import os
 import re
@@ -73,6 +74,25 @@ def render_skill_md(
     return "\n".join(chunks)
 
 
+def _is_symlink_privilege_error(exc: OSError) -> bool:
+    """Return True only for the Windows 'symlink privilege not held' case.
+
+    We must not mask a real collision/error by silently falling back to a copy;
+    only the case where the OS refuses to create a symlink because the caller
+    lacks SeCreateSymbolicLinkPrivilege (Windows Developer Mode / elevation)
+    should fall back to a copy inside a private work dir.
+    """
+    if getattr(exc, "winerror", None) in (1314,):  # ERROR_PRIVILEGE_NOT_HELD
+        return True
+    if isinstance(exc, OSError):
+        return exc.errno in {
+            getattr(errno, "EPERM", -1),
+            getattr(errno, "ENOTSUP", -1),
+            getattr(errno, "EOPNOTSUPP", -1),
+        }
+    return False
+
+
 def prepare_workspace(
     *,
     work_dir: str,
@@ -120,7 +140,22 @@ def prepare_workspace(
             parent = os.path.dirname(dst)
             if parent:
                 os.makedirs(parent, exist_ok=True)
-            os.symlink(os.path.abspath(src), dst)
+            src_abs = os.path.abspath(src)
+            if os.path.lexists(dst):
+                raise FileExistsError(
+                    f"link destination already exists: {dst} (from {src})"
+                )
+            try:
+                os.symlink(src_abs, dst, target_is_directory=os.path.isdir(src_abs))
+            except OSError as exc:
+                # Fail closed: only fall back for the Windows symlink-privilege
+                # case, and never merge into an existing destination.
+                if not _is_symlink_privilege_error(exc):
+                    raise
+                if os.path.isdir(src_abs):
+                    shutil.copytree(src_abs, dst)
+                else:
+                    shutil.copy2(src_abs, dst)
 
     attachment_lines: list[str] = []
     if images:
@@ -1505,6 +1540,56 @@ def _sanitize_cursor_json(value: Any, *, field: str = "") -> Any:
     return value
 
 
+def _redact_copilot_json(value: Any, *, field: str = "") -> Any:
+    """Mapping-key-aware redaction for Copilot JSONL.
+
+    Unlike the cursor trace sanitizer, this does NOT omit ``content``/``prompt``
+    (those are the CLI output we want to keep debuggable); it redacts by secret
+    field name and applies the string-level redactor to remaining string leaves.
+    """
+    normalized_field = re.sub(r"[^a-z0-9]", "", field.lower())
+    if (
+        normalized_field in _CURSOR_SECRET_TRACE_FIELDS
+        or normalized_field.endswith("apikey")
+    ):
+        return "[REDACTED]"
+    if isinstance(value, dict):
+        return {
+            str(key): _redact_copilot_json(item, field=str(key))
+            for key, item in value.items()
+        }
+    if isinstance(value, list):
+        return [_redact_copilot_json(item) for item in value]
+    if isinstance(value, str):
+        return _redact_cursor_error(value)
+    return value
+
+
+def _redact_copilot_trace(raw: str | bytes) -> str:
+    """Structurally sanitize Copilot JSONL output (mapping-key aware).
+
+    ``_redact_cursor_error`` catches unquoted ``key=value`` forms, but the
+    Copilot JSONL stream carries ``"token": "..."`` objects. Parse each line
+    and walk it with the field-name-aware sanitizer; non-JSON lines still fall
+    through the string-level redactor.
+    """
+    text = _cursor_process_text(raw)
+    lines_out: list[str] = []
+    for line in text.splitlines():
+        stripped = line.strip()
+        if not stripped:
+            lines_out.append(line)
+            continue
+        try:
+            obj = json.loads(stripped)
+        except (ValueError, TypeError):
+            lines_out.append(_redact_cursor_error(line))
+            continue
+        obj = _redact_copilot_json(obj)
+        lines_out.append(json.dumps(obj, ensure_ascii=False))
+    return "\n".join(lines_out)
+
+
 def _cursor_process_text(value: str | bytes) -> str:
     if isinstance(value, bytes):
         return value.decode("utf-8", errors="replace")
@@ -1771,14 +1856,15 @@ def run_copilot_exec(
 
         stdout = proc.stdout or ""
         stderr = proc.stderr or ""
-        safe_raw = stdout
+        safe_raw = _redact_copilot_trace(stdout)
         if stderr:
-            safe_raw = f"{safe_raw}\n[stderr]\n{stderr}" if safe_raw else f"[stderr]\n{stderr}"
+            safe_stderr = _redact_copilot_trace(stderr)
+            safe_raw = f"{safe_raw}\n[stderr]\n{safe_stderr}" if safe_raw else f"[stderr]\n{safe_stderr}"
         all_raw.append(f"===== COPILOT CLI ATTEMPT {attempt + 1} =====\n{safe_raw}")
         combined = "\n\n".join(all_raw)
 
         if proc.returncode != 0:
-            detail = (stderr or stdout).strip()[:4000]
+            detail = _redact_copilot_trace((stderr or stdout).strip())[:4000]
             raise RuntimeError(
                 f"Copilot CLI failed with exit code {proc.returncode}: {detail}"
             )

tests/test_cli_path_resolution.py

@@ -0,0 +1,29 @@
+"""Tests for CLI exec-path resolution (Windows bare-name .cmd shims)."""
+
+from __future__ import annotations
+
+from skillopt.model.backend_config import _resolve_cli_path
+
+
+def test_resolve_cli_path_uses_shutil_which(monkeypatch):
+    """A name found on PATH resolves to its real executable."""
+    monkeypatch.setattr("shutil.which", lambda v: f"/resolved/{v}")
+    assert _resolve_cli_path("codex") == "/resolved/codex"
+
+
+def test_resolve_cli_path_falls_back_to_original(monkeypatch):
+    """A name not on PATH (or a bare name on a host without it) passes through."""
+    monkeypatch.setattr("shutil.which", lambda v: None)
+    assert _resolve_cli_path("codex") == "codex"
+
+
+def test_resolve_cli_path_keeps_configured_absolute_path(monkeypatch):
+    """An absolute configured path that cannot be resolved is preserved."""
+    monkeypatch.setattr("shutil.which", lambda v: None)
+    assert _resolve_cli_path("/opt/bin/codex") == "/opt/bin/codex"
+
+
+def test_resolve_cli_path_not_called_with_empty(monkeypatch):
+    """Empty input is not handed to shutil.which in a way that corrupts."""
+    monkeypatch.setattr("shutil.which", lambda v: None)
+    assert _resolve_cli_path("") == ""

tests/test_workspace_symlink.py

@@ -0,0 +1,131 @@
+"""Tests for workspace-prep symlink fail-closed behavior + Copilot trace redaction."""
+
+from __future__ import annotations
+
+import errno
+import os
+from pathlib import Path
+
+import pytest
+
+from skillopt.model.codex_harness import (
+    _is_symlink_privilege_error,
+    _redact_copilot_trace,
+    prepare_workspace,
+)
+
+
+def _mk_src(tmp_path, name="srcdir", content="source-content") -> Path:
+    src = tmp_path / name
+    src.mkdir()
+    (src / "data.txt").write_text(content, encoding="utf-8")
+    return src
+
+
+def _privilege_error() -> OSError:
+    e = OSError()
+    e.winerror = 1314  # ERROR_PRIVILEGE_NOT_HELD
+    return e
+
+
+def test_is_symlink_privilege_error():
+    assert _is_symlink_privilege_error(_privilege_error()) is True
+    e = OSError()
+    e.errno = errno.EPERM
+    assert _is_symlink_privilege_error(e) is True
+    e2 = OSError()
+    e2.errno = errno.EACCES
+    assert _is_symlink_privilege_error(e2) is False
+    assert _is_symlink_privilege_error(OSError()) is False
+
+
+def test_symlink_privilege_fallback_copies_and_leaves_source(monkeypatch, tmp_path):
+    src = _mk_src(tmp_path)
+    work = tmp_path / "work"
+    work.mkdir()
+
+    def fake_symlink(a, b, **kw):
+        raise _privilege_error()
+
+    monkeypatch.setattr("skillopt.model.codex_harness.os.symlink", fake_symlink)
+    prepare_workspace(work_dir=str(work), skill_md="x", link_dirs=[(str(src), "docs/data")])
+
+    # Source is not modified.
+    assert (src / "data.txt").read_text(encoding="utf-8") == "source-content"
+    # The fallback copied into the private work dir.
+    assert (work / "docs" / "data" / "data.txt").read_text(encoding="utf-8") == "source-content"
+
+
+def test_existing_destination_fails_closed(monkeypatch, tmp_path):
+    src = _mk_src(tmp_path)
+    work = tmp_path / "work"
+    work.mkdir()
+
+    # extra_files creates the destination directory before link_dirs runs.
+    with pytest.raises(FileExistsError):
+        prepare_workspace(
+            work_dir=str(work),
+            skill_md="x",
+            extra_files={"docs/data/file.txt": "stale"},
+            link_dirs=[(str(src), "docs/data")],
+        )
+    # extra_files content was not overwritten by a merge.
+    assert (work / "docs" / "data" / "file.txt").read_text(encoding="utf-8") == "stale"
+
+
+def test_duplicate_destination_fails_closed(monkeypatch, tmp_path):
+    src_a = _mk_src(tmp_path, "A")
+    src_b = _mk_src(tmp_path, "B", content="B-content")
+    work = tmp_path / "work"
+    work.mkdir()
+
+    # Force the first to succeed via a dir symlink stub, then assert the second
+    # duplicate destination is refused rather than merged.
+    def fake_symlink(a, b, **kw):
+        Path(b).mkdir(parents=True, exist_ok=True)
+
+    monkeypatch.setattr("skillopt.model.codex_harness.os.symlink", fake_symlink)
+    with pytest.raises(FileExistsError):
+        prepare_workspace(
+            work_dir=str(work),
+            skill_md="x",
+            link_dirs=[(str(src_a), "shared"), (str(src_b), "shared")],
+        )
+    # src_b (the second source) must not have been copied over the first.
+    assert not (work / "shared" / "data.txt").exists() or (
+        src_b / "data.txt"
+    ).read_text(encoding="utf-8") == "B-content"
+
+
+def test_non_privilege_symlink_error_reraises(monkeypatch, tmp_path):
+    src = _mk_src(tmp_path)
+    work = tmp_path / "work"
+    work.mkdir()
+
+    def fake_symlink(a, b, **kw):
+        e = OSError()
+        e.errno = errno.EACCES
+        raise e
+
+    monkeypatch.setattr("skillopt.model.codex_harness.os.symlink", fake_symlink)
+    with pytest.raises(OSError):
+        prepare_workspace(work_dir=str(work), skill_md="x", link_dirs=[(str(src), "docs/data")])
+
+
+def test_redact_copilot_trace_redacts_json_secret_fields():
+    line = '{"type":"message","token":"plain-secret","content":"hi"}'
+    out = _redact_copilot_trace(line)
+    assert "plain-secret" not in out
+    # Non-secret fields survive (content is an omitted trace field by design).
+    assert "message" in out
+
+
+def test_redact_copilot_trace_preserves_normal_strings():
+    out = _redact_copilot_trace('{"type":"assistant","message":"hello"}')
+    # Normal field names are preserved.
+    assert "hello" in out
+
+
+def test_redact_copilot_trace_redacts_string_key_forms():
+    out = _redact_copilot_trace("token = plain-secret")
+    assert "plain-secret" not in out