diff --git a/skillopt/model/backend_config.py b/skillopt/model/backend_config.py index 8b84d213..61a0ec27 100644 --- a/skillopt/model/backend_config.py +++ b/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() diff --git a/skillopt/model/codex_harness.py b/skillopt/model/codex_harness.py index 70bb7151..14802980 100644 --- a/skillopt/model/codex_harness.py +++ b/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}" ) diff --git a/tests/test_cli_path_resolution.py b/tests/test_cli_path_resolution.py new file mode 100644 index 00000000..af30aca8 --- /dev/null +++ b/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("") == "" diff --git a/tests/test_workspace_symlink.py b/tests/test_workspace_symlink.py new file mode 100644 index 00000000..e736cc49 --- /dev/null +++ b/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