github-featured/microsoft--skillopt
2
0
Fork 0
Code Issues Pull Requests Deployments Models Agent Notes Packages Projects Releases Wiki Activity

fix(model): consolidate Copilot review cleanup

Browse Source 
Clarify null and cloud-service documentation, document the tool-call limitation, harden and deduplicate JSONL parsing, centralize child-environment sanitization, normalize legacy backend selection, and replace source-text CLI checks with behavioral assertions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 0e8472e4-56ad-4daf-80b4-1c0ed0258133
This commit is contained in:
Christopher Haugen
2026-08-06 10:22:19 +02:00
parent 4b4d9c568e
commit 5497a3123a
9 changed files with 92 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+5 -3
View File
@@ -10,9 +10,11 @@ All notable changes to SkillOpt are documented here. This project adheres to
- **GitHub Copilot CLI backend**, in two forms: `copilot_chat` (usable as both
optimizer and target) and `copilot_exec` (target-only execution harness).
Because the Copilot CLI carries its own sign-in, `--backend copilot` selects
`copilot_chat` for both roles and runs a complete train/eval loop with **no
cloud API key**. Calls disable built-in MCP servers and custom instructions,
and never pass `--allow-all-tools`; `copilot_exec` requires an explicit
`copilot_chat` for both roles and runs a complete train/eval loop with no
separate provider API key; inference still uses the GitHub Copilot cloud
service. Chat calls disable all built-in tools, built-in MCP servers, and
custom instructions, strip inherited `COPILOT_ALLOW_ALL`, and never pass
`--allow-all-tools`; `copilot_exec` requires an explicit
`copilot_exec_allow_all_tools` opt-in before granting unattended tool use.
The CLI reports no token counts, so usage totals are zero for these backends.
- A non-destructive Devin installer and SessionEnd activity marker, preserving
configs/_base_/default.yaml
+2 -2
View File
@@ -30,8 +30,8 @@ model:
# using CLI authentication rather than a separate provider API key.
copilot_exec_path: "" # blank uses COPILOT_EXEC_PATH or copilot
copilot_exec_home: "" # blank leaves COPILOT_HOME untouched
copilot_exec_allow_all_tools: null # blank uses COPILOT_EXEC_ALLOW_ALL_TOOLS (default off);
# copilot_exec only, required for file-edit rollouts
# null preserves COPILOT_EXEC_ALLOW_ALL_TOOLS (default off); copilot_exec only
copilot_exec_allow_all_tools: null # required for file-edit rollouts
copilot_chat_optimizer_model: "" # blank lets the CLI pick its default model
copilot_chat_target_model: ""
copilot_chat_timeout: 0 # 0 uses COPILOT_CHAT_TIMEOUT or the built-in default
docs/guide/configuration.md
+4 -2
View File
@@ -56,6 +56,8 @@ cloud service. Sign in once with `copilot` (GitHub Copilot CLI) beforehand.
Expect roughly 20-40 s per call: the CLI is an agent, not a completions
endpoint, so a full-size run is far slower than a hosted backend. It also
reports no token counts, so usage totals are zero.
It does not support caller-supplied tools or structured tool calls; environments
that require those features must use another chat backend.
The current MiniMax adapter has one shared deployment. Set
`model.minimax_model` when MiniMax is the target; a mixed-backend run cannot
@@ -230,8 +232,8 @@ Because the CLI carries its own sign-in, selecting it for both roles
(`--backend copilot`) avoids separate provider API-key configuration:
```bash
copilot # sign in once, then exit
skillopt-train --config configs/train/default.yaml --backend copilot
copilot login
skillopt-train --config configs/searchqa/default.yaml --backend copilot
```
Chat calls use an empty tool allowlist and disable built-in MCP servers and
docs/reference/config.md
+1 -1
View File
@@ -71,7 +71,7 @@ defaults to `claude` and can be overridden with `CLAUDE_CLI_BIN`.
| `model.cursor_exec_sandbox` | Cursor sandbox mode: `enabled` (default) or `disabled`; file-edit rollouts require `enabled` |
| `model.copilot_exec_path` | GitHub Copilot CLI executable path; default `copilot` |
| `model.copilot_exec_home` | Optional `COPILOT_HOME` override isolating CLI config |
| `model.copilot_exec_allow_all_tools` | Opt in to `--allow-all-tools`; `false` by default, required for file-edit rollouts |
| `model.copilot_exec_allow_all_tools` | Optional opt-in to `--allow-all-tools`; unset by default so `COPILOT_EXEC_ALLOW_ALL_TOOLS` remains authoritative |
| `model.copilot_chat_optimizer_model` / `model.copilot_chat_target_model` | Optional per-role `--model` IDs for `copilot_chat` |
| `model.copilot_chat_timeout` | Per-call timeout in seconds for `copilot_chat` |
skillopt/engine/trainer.py
+4 -4
View File
@@ -464,7 +464,7 @@ def _resolve_role_backends(
something else always wins.
"""
backend = normalize_backend_name(backend)
if backend in {"claude", "claude_chat"}:
if backend == "claude_chat":
# A chat backend fills BOTH roles, so -- like copilot -- a role pinned
# to a default (including the base config's truthy openai_chat) must be
# overridden. `x = x or ...` would leave openai_chat in place.
@@ -481,11 +481,11 @@ def _resolve_role_backends(
optimizer_backend = optimizer_backend or "openai_chat"
if target_backend in _ROLE_BACKEND_DEFAULTS:
target_backend = "claude_code_exec"
elif backend in {"cursor", "cursor_exec"}:
elif backend == "cursor_exec":
optimizer_backend = optimizer_backend or "openai_chat"
if target_backend in _ROLE_BACKEND_DEFAULTS:
target_backend = "cursor_exec"
elif backend in {"copilot", "copilot_chat"}:
elif backend == "copilot_chat":
# Both roles use the locally installed, CLI-authenticated backend.
if optimizer_backend in _ROLE_BACKEND_DEFAULTS:
optimizer_backend = "copilot_chat"
@@ -495,7 +495,7 @@ def _resolve_role_backends(
optimizer_backend = optimizer_backend or "openai_chat"
if target_backend in _ROLE_BACKEND_DEFAULTS:
target_backend = "copilot_exec"
elif backend in {"qwen", "qwen_chat"}:
elif backend == "qwen_chat":
optimizer_backend = optimizer_backend or "openai_chat"
if target_backend in _ROLE_BACKEND_DEFAULTS:
target_backend = "qwen_chat"
skillopt/model/__init__.py
+10 -13
View File
@@ -30,6 +30,7 @@ from skillopt.model.backend_config import ( # noqa: F401
set_optimizer_backend,
set_target_backend,
)
from skillopt.model.common import normalize_backend_name
def set_backend(name: str | None) -> str:
@@ -39,20 +40,16 @@ def set_backend(name: str | None) -> str:
target. Keep that entry point so older scripts continue to work, while
mapping it onto the split optimizer/target backend model.
"""
normalized = str(name or "azure_openai").strip().lower()
if normalized in {"azure_openai", "openai_chat", "azure", "azure-openai"}:
normalized = normalize_backend_name(name)
if normalized in {"azure_openai", "openai_chat"}:
set_optimizer_backend("openai_chat")
set_target_backend("openai_chat")
return "azure_openai"
if normalized in {"claude", "claude_chat", "anthropic"}:
if normalized == "claude_chat":
set_optimizer_backend("claude_chat")
set_target_backend("claude_chat")
return "claude_chat"
if normalized == "codex":
set_optimizer_backend("codex_exec")
set_target_backend("codex_exec")
return "codex"
if normalized == "codex_exec":
if normalized in {"codex", "codex_exec"}:
set_optimizer_backend("codex_exec")
set_target_backend("codex_exec")
return normalized
@@ -60,11 +57,11 @@ def set_backend(name: str | None) -> str:
set_optimizer_backend("openai_chat")
set_target_backend(normalized)
return normalized
if normalized in {"cursor", "cursor_agent", "cursor_exec"}:
if normalized == "cursor_exec":
set_optimizer_backend("openai_chat")
set_target_backend("cursor_exec")
return "cursor_exec"
if normalized in {"copilot", "copilot_cli", "github_copilot", "copilot_chat"}:
if normalized == "copilot_chat":
# The CLI-authenticated backend drives both roles without a separate
# provider API key; inference still uses the Copilot cloud service.
set_optimizer_backend("copilot_chat")
@@ -74,15 +71,15 @@ def set_backend(name: str | None) -> str:
set_optimizer_backend("openai_chat")
set_target_backend("copilot_exec")
return "copilot_exec"
if normalized in {"qwen", "qwen_chat"}:
if normalized == "qwen_chat":
set_optimizer_backend("openai_chat")
set_target_backend("qwen_chat")
return "qwen_chat"
if normalized in {"minimax", "minimax_chat"}:
if normalized == "minimax_chat":
set_optimizer_backend("openai_chat")
set_target_backend("minimax_chat")
return "minimax_chat"
if normalized in {"openai_compatible", "openai_compatible_chat", "openai-compatible", "compat"}:
if normalized == "openai_compatible":
set_optimizer_backend("openai_compatible")
set_target_backend("openai_compatible")
return "openai_compatible"
skillopt/model/codex_harness.py
+6 -25
View File
@@ -18,6 +18,10 @@ from skillopt.model.backend_config import (
get_cursor_exec_config,
get_target_backend,
)
from skillopt.model.copilot_backend import (
build_copilot_subprocess_env,
parse_copilot_jsonl,
)
ANSWER_SCHEMA: dict[str, Any] = {
"type": "object",
@@ -1360,13 +1364,8 @@ def run_copilot_exec(
for path in add_dirs:
cmd.extend(["--add-dir", path])
env = os.environ.copy()
# Configuration is authoritative: an inherited CLI-wide opt-in must
# not bypass SkillOpt's allow_all_tools and allow_file_edits gates.
env.pop("COPILOT_ALLOW_ALL", None)
home = str(config.get("home") or "")
if home:
env["COPILOT_HOME"] = home
env = build_copilot_subprocess_env(home)
try:
proc = subprocess.run(
@@ -1400,7 +1399,7 @@ def run_copilot_exec(
f"Copilot CLI failed with exit code {proc.returncode}: {detail}"
)
response = _parse_copilot_jsonl(stdout)
response = parse_copilot_jsonl(stdout)
if response:
return response, combined
@@ -1412,24 +1411,6 @@ def run_copilot_exec(
raise RuntimeError(f"{last_error}\n{detail}" if detail else last_error)
def _parse_copilot_jsonl(raw: str) -> str:
"""Concatenate ``assistant.message`` content from a Copilot JSONL stream."""
parts: list[str] = []
for line in (raw or "").splitlines():
line = line.strip()
if not line or not line.startswith("{"):
continue
try:
obj = json.loads(line)
except Exception:
continue
if obj.get("type") == "assistant.message":
content = (obj.get("data") or {}).get("content")
if isinstance(content, str) and content:
parts.append(content)
return "\n".join(parts).strip()
def run_target_exec(
*,
work_dir: str,
skillopt/model/copilot_backend.py
+20 -10
View File
@@ -32,6 +32,15 @@ tracker = TokenTracker()
_ZERO_USAGE = {"prompt_tokens": 0, "completion_tokens": 0, "total_tokens": 0}
def build_copilot_subprocess_env(home: str = "") -> dict[str, str]:
"""Build a child environment without inherited unattended-tool approval."""
env = os.environ.copy()
env.pop("COPILOT_ALLOW_ALL", None)
if home:
env["COPILOT_HOME"] = home
return env
def _compose_prompt(system: str, user: str) -> str:
system = (system or "").strip()
user = (user or "").strip()
@@ -56,7 +65,8 @@ def _messages_to_prompt(messages: list[dict[str, Any]]) -> str:
return "\n\n---\n\n".join(parts)
def _parse_jsonl_response(raw: str) -> str:
def parse_copilot_jsonl(raw: str) -> str:
"""Concatenate assistant text from a Copilot JSONL event stream."""
parts: list[str] = []
for line in (raw or "").splitlines():
line = line.strip()
@@ -64,10 +74,15 @@ def _parse_jsonl_response(raw: str) -> str:
continue
try:
obj = json.loads(line)
except Exception:
except json.JSONDecodeError:
continue
if not isinstance(obj, dict):
continue
if obj.get("type") == "assistant.message":
content = (obj.get("data") or {}).get("content")
data = obj.get("data")
if not isinstance(data, dict):
continue
content = data.get("content")
if isinstance(content, str) and content:
parts.append(content)
return "\n".join(parts).strip()
@@ -96,13 +111,8 @@ def _invoke(prompt: str, *, model: str, timeout: float | None) -> str:
if chosen:
cmd.extend(["--model", chosen])
env = os.environ.copy()
# The parent may use this for its own Copilot session. It must not silently
# turn a SkillOpt child into an unrestricted agent.
env.pop("COPILOT_ALLOW_ALL", None)
home = str(config.get("home") or "")
if home:
env["COPILOT_HOME"] = home
env = build_copilot_subprocess_env(home)
proc = subprocess.run(
cmd,
@@ -116,7 +126,7 @@ def _invoke(prompt: str, *, model: str, timeout: float | None) -> str:
if proc.returncode != 0:
detail = ((proc.stderr or proc.stdout) or "").strip()[:4000]
raise RuntimeError(f"Copilot CLI failed with exit code {proc.returncode}: {detail}")
return _parse_jsonl_response(proc.stdout or "")
return parse_copilot_jsonl(proc.stdout or "")
def _chat_impl(
tests/test_copilot_exec_backend.py
+40 -14
View File
@@ -1,8 +1,10 @@
from __future__ import annotations
import ast
import json
import os
import subprocess
import sys
from collections.abc import Iterator
from pathlib import Path
@@ -65,10 +67,13 @@ def test_aliases_normalize_to_copilot_exec(alias: str) -> None:
@pytest.mark.parametrize("alias", ["copilot", "copilot_cli", "github_copilot"])
def test_bare_copilot_aliases_normalize_to_the_fully_local_chat_backend(
def test_bare_copilot_aliases_normalize_to_the_cli_authenticated_chat_backend(
alias: str,
) -> None:
assert normalize_backend_name(alias) == "copilot_chat"
assert model.set_backend(alias) == "copilot_chat"
assert backend_config.get_optimizer_backend() == "copilot_chat"
assert backend_config.get_target_backend() == "copilot_chat"
def test_set_backend_routes_target_to_copilot_and_keeps_chat_optimizer() -> None:
@@ -119,16 +124,19 @@ def test_parse_jsonl_concatenates_assistant_messages_and_ignores_noise() -> None
json.dumps({"type": "tool.call", "data": {"content": "ignored"}}),
json.dumps({"type": "assistant.message", "data": {"content": "first"}}),
"{ broken json",
"[]",
"null",
json.dumps({"type": "assistant.message", "data": "invalid"}),
json.dumps({"type": "assistant.message", "data": {"content": "second"}}),
json.dumps({"type": "assistant.message", "data": {}}),
]
)
assert harness._parse_copilot_jsonl(raw) == "first\nsecond"
assert copilot_backend.parse_copilot_jsonl(raw) == "first\nsecond"
def test_parse_jsonl_returns_empty_for_no_assistant_messages() -> None:
assert harness._parse_copilot_jsonl('{"type":"tool.call","data":{}}') == ""
assert harness._parse_copilot_jsonl("") == ""
assert copilot_backend.parse_copilot_jsonl('{"type":"tool.call","data":{}}') == ""
assert copilot_backend.parse_copilot_jsonl("") == ""
def _fake_run(captured: dict, stdout: str, returncode: int = 0):
@@ -216,7 +224,8 @@ def test_copilot_backends_keep_a_real_deployment_fallback() -> None:
assert default_model_for_backend("copilot_chat") == "gpt-4o"
def test_no_response_error_includes_cli_output(monkeypatch, tmp_path) -> None: # copilot_exec persists no artifacts, so a bare "returned no response"
def test_no_response_error_includes_cli_output(monkeypatch, tmp_path) -> None:
# copilot_exec persists no artifacts, so a bare "returned no response"
# would leave an empty/invalid JSONL stream undebuggable.
model.set_backend("copilot")
model.configure_copilot_exec(path="copilot", home="", allow_all_tools=False)
@@ -459,19 +468,36 @@ def test_copilot_config_keys_survive_flattening() -> None:
@pytest.mark.parametrize("script", ["train", "eval_only"])
def test_cli_exposes_copilot_backend_and_flags(script: str) -> None:
root = Path(__file__).resolve().parents[1]
text = (root / "scripts" / f"{script}.py").read_text(encoding="utf-8")
assert "copilot_chat" in text
assert "copilot_exec" in text
script_path = root / "scripts" / f"{script}.py"
result = subprocess.run(
[sys.executable, str(script_path), "--help"],
capture_output=True,
text=True,
timeout=30,
)
assert result.returncode == 0, result.stderr
assert "copilot_chat" in result.stdout
assert "copilot_exec" in result.stdout
for flag in (
"--copilot_exec_path",
"--copilot_exec_home",
"--copilot_exec_allow_all_tools",
"--copilot_chat_optimizer_model",
"--copilot_chat_target_model",
"--copilot_chat_timeout",
):
assert flag in text
assert flag in result.stdout
# train.py delegates backend configuration to the trainer; eval_only wires
# it directly. Assert whichever applies so the wiring can't silently drop.
applier = (
text if script == "eval_only" else (root / "skillopt" / "engine" / "trainer.py").read_text(encoding="utf-8")
# it directly. Inspect call nodes rather than source spelling/formatting.
applier_path = (
script_path
if script == "eval_only"
else root / "skillopt" / "engine" / "trainer.py"
)
assert "configure_copilot_chat" in applier
assert "configure_copilot_exec" in applier
tree = ast.parse(applier_path.read_text(encoding="utf-8"))
calls = {
node.func.id
for node in ast.walk(tree)
if isinstance(node, ast.Call) and isinstance(node.func, ast.Name)
}
assert {"configure_copilot_chat", "configure_copilot_exec"} <= calls