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

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

- Resolve codex/claude/cursor/copilot exec paths via shutil.which at config
  load so bare npm .cmd shims spawn on Windows (bare 'codex' -> codex.CMD;
  CreateProcess does not search PATHEXT, so bare names raise WinError 2).
- os.symlink uses target_is_directory + copytree/copy2 fallback (OfficeQA
  on Windows).
- Add tests for _resolve_cli_path.

* fix(model): fail closed on symlink collisions; redact Copilot JSONL

Address maintainer review on #250:
- prepare_workspace symlink fallback fails closed: re-raise when the
  destination already exists (lexists), never copytree into an existing dst,
  and only fall back for the Windows symlink-privilege-not-held error.
  Prevents a colliding extra_files or duplicate link_dirs entry from mutating
  source data outside the work dir.
- Sanitize Copilot stdout/stderr structurally (mapping-key aware) so JSON
  objects carrying a token/secret field are redacted even when the value is
  a quoted mapping (which the string-level redactor missed).
- Add tests: symlink privilege fallback, existing/duplicate dst, non-privilege
  re-raise, and Copilot JSONL secret-field redaction.

* fix(model): key-aware redaction of quoted JSON in copilot/cursor traces

- Fold quoted-JSON whole-value capture into the shared _redact_cursor_error
  (run first) so a quoted value with spaces is scrubbed whole instead of
  truncating at the first space and leaking the remainder. One change covers
  the copilot fallback, the cursor stderr paths, and all string-leaf callers.
- Regressions: prefixed/multiline quoted JSON, space-in-value, cursor parity.

* fix(model): endswith-based secret-key detection in quoted-JSON redaction

- The substring regex over-scrubbed diagnostics like token_count / token_budget /
  secret_version. Decide with the project's endswith-based rule (matching
  _is_secret_mapping_key) so only real credential keys are redacted, while a
  quoted value that contains spaces is still captured whole.
- Regressions: token-count diagnostic retention + nested secret key.

* fix(model): also treat bare "bearer" key as a credential

- Add "bearer" to the exact secret-key set so a standalone "bearer": <value>
  JSON key is scrubbed (the common "Authorization": "Bearer ..." form was
  already covered via the authorization key).
- Regression: bare bearer key redaction.

* fix(model): unify copilot redaction key policy + unbounded embedded-JSON parsing

- _redact_copilot_json now uses the same endswith-based _is_copilot_secret_key as
  the embedded-JSON fallback, so camelCase keys like githubToken are redacted.
- Replace the single-level regex and line-by-line parsing with a bracket-matched,
  unbounded-nesting embedded-JSON scan, so deeply nested / pretty-printed JSON in
  non-JSON text is structurally redacted.
- Add run-level regressions (success trace + error detail) for camelCase keys and
  deep-nested embedded JSON.

---------

Co-authored-by: WODE25500 <WODE25500@users.noreply.github.com>

Author: WODE25500

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:
@@ -1479,8 +1514,115 @@ def run_codex_exec(
 }
 
 
+# ``"token": "..."`` / ``"accessToken": {...}`` quoted JSON pairs, matched in
+# arbitrary (possibly non-JSON) text. Value may be a string, number, bool, or a
+# nested object/array literal quoted as a unit — we redact the whole payload.
+# Applied FIRST inside ``_redact_cursor_error``: the unquoted keyword regex below
+# stops at the first whitespace, so ``"token": "a b c"`` would otherwise leak
+# ``b c"``. Capturing the whole quoted payload up front fixes that, and since
+# ``_redact_cursor_error`` is the single shared string redactor, one change
+# covers the copilot fallback, the cursor stderr paths, and string leaves.
+# ponytail: the object branch is single-level only; deep-nested values under a
+# secret key in non-JSON text are not stripped (valid-JSON lines already go
+# through the structural walker). Add an unbounded nest parser if that ever
+# appears in real stderr.
+_COPILOT_SECRET_KEY_SUFFIXES = (
+    "apikey",
+    "accesstoken",
+    "refreshtoken",
+    "token",
+    "password",
+    "passwd",
+    "clientsecret",
+    "secret",
+    "secretkey",
+    "secretaccesskey",
+    "sharedaccesskey",
+    "privatekey",
+    "accountkey",
+    "cookie",
+    "setcookie",
+)
+_COPILOT_SECRET_KEY_EXACT = {"pwd", "sig", "authorization", "bearer"}
+
+def _is_copilot_secret_key(field: str) -> bool:
+    """The single mapping-aware secret-key policy for the Copilot path.
+
+    Uses endswith on the compacted key (mirroring ``_is_secret_mapping_key``), so
+    ``token`` / ``api_key`` / ``refreshToken`` / ``bearer`` / ``cookie`` are
+    redacted, but ``token_count`` / ``token_budget`` / ``secret_version``
+    diagnostics are preserved.
+    """
+    compact = re.sub(r"[^a-z0-9]", "", (field or "").casefold())
+    return compact in _COPILOT_SECRET_KEY_EXACT or compact.endswith(_COPILOT_SECRET_KEY_SUFFIXES)
+
+
+def _find_json_end(text: str, start: int) -> int | None:
+    """Bracket-match a JSON object/array starting at ``start`` (unbounded nesting).
+
+    String-aware (handles quotes and escapes), so a ``{`` inside a string value
+    does not confuse the matching.
+    """
+    open_ch = text[start]
+    close_ch = "}" if open_ch == "{" else "]"
+    depth = 0
+    in_str = False
+    escaped = False
+    for k in range(start, len(text)):
+        ch = text[k]
+        if in_str:
+            if escaped:
+                escaped = False
+            elif ch == "\\":
+                escaped = True
+            elif ch == '"':
+                in_str = False
+        else:
+            if ch == '"':
+                in_str = True
+            elif ch == open_ch:
+                depth += 1
+            elif ch == close_ch:
+                depth -= 1
+                if depth == 0:
+                    return k
+    return None
+
+
+def _redact_embedded_json(text: str) -> str:
+    """Structurally redact JSON objects/arrays embedded in plain text.
+
+    Finds balanced JSON fragments (unbounded nesting, string-aware) and walks
+    each with the mapping-aware redactor, so deeply nested or pretty-printed
+    JSON embedded in a non-JSON line no longer leaks. Non-JSON text is preserved.
+    """
+    out: list[str] = []
+    i = 0
+    n = len(text)
+    while i < n:
+        ch = text[i]
+        if ch in "{[":
+            end = _find_json_end(text, i)
+            if end is not None:
+                frag = text[i:end + 1]
+                try:
+                    obj = json.loads(frag)
+                except (ValueError, TypeError):
+                    out.append(ch)
+                    i += 1
+                    continue
+                out.append(json.dumps(_redact_copilot_json(obj), ensure_ascii=False))
+                i = end + 1
+                continue
+        out.append(ch)
+        i += 1
+    return "".join(out)
+
+
 def _redact_cursor_error(value: str) -> str:
-    text = _CURSOR_SECRET_ASSIGNMENT.sub(r"\1\2[REDACTED]", value or "")
+    text = value or ""
+    text = _redact_embedded_json(text)
+    text = _CURSOR_SECRET_ASSIGNMENT.sub(r"\1\2[REDACTED]", text)
     return _CURSOR_SECRET_TOKEN.sub("[REDACTED]", text)
 
 
@@ -1505,6 +1647,42 @@ 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.
+    Uses the SAME key policy as the embedded-JSON fallback so valid JSON and
+    non-JSON fragments agree on what a secret field is.
+    """
+    if _is_copilot_secret_key(field):
+        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:
+    """Sanitize Copilot JSONL output (mapping-key aware, unbounded nesting).
+
+    The whole text is scanned for JSON objects/arrays (single-line, multiple
+    fragments, or pretty-printed / deeply nested) and each is walked with the
+    mapping-aware redactor; remaining non-JSON text gets string-level redaction
+    for ``key=value`` and token patterns. This replaces the old line-by-line
+    regex, which only handled single-level object values.
+    """
+    text = _cursor_process_text(raw)
+    return _redact_cursor_error(text)
+
+
 def _cursor_process_text(value: str | bytes) -> str:
     if isinstance(value, bytes):
         return value.decode("utf-8", errors="replace")
@@ -1771,14 +1949,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("") == ""