Skip to content

 feat(model): add the GitHub Copilot CLI as a backend - #202

Merged
Yifan Yang (Yif-Yang) merged 19 commits into
microsoft:mainfrom
lufen:feat/copilot-cli-backend
Aug 6, 2026
Merged

 feat(model): add the GitHub Copilot CLI as a backend#202
Yifan Yang (Yif-Yang) merged 19 commits into
microsoft:mainfrom
lufen:feat/copilot-cli-backend

Conversation

@lufen

@lufen Christopher Haugen (lufen) commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Adds two backends. copilot_chat drives the Copilot CLI as a chat model and
can fill either role, so --backend copilot selects it for BOTH optimizer and
target. The CLI carries its own sign-in, enabling a complete train/eval loop
with no separate provider API key; inference still uses the GitHub Copilot
cloud service. copilot_exec is the separate target-only execution harness,
alongside the existing codex/claude/cursor harnesses.

Verified end to end on SearchQA with the CLI signed in and no separate provider
API credentials configured: baseline eval, rollout, reflect, aggregate, select,
update and gate all execute through the locally installed CLI.

Safety: chat calls disable all built-in tools, built-in MCP servers, and custom
instructions so the model sees only the prompt SkillOpt sends. Child processes
strip inherited COPILOT_ALLOW_ALL, and chat calls never pass
--allow-all-tools. Unlike the other exec harnesses, copilot_exec does NOT
grant unattended tool use by default — it requires an explicit
copilot_exec_allow_all_tools opt-in, because a file-edit rollout is the only
case that needs it.

Two caveats worth knowing before use: the CLI is an agent rather than a
completions endpoint, so expect roughly 20–40 s per call; and it reports no
token counts, so usage totals are zero for these backends.

…backends

configs/_base_/default.yaml ships optimizer_backend: openai_chat and
	arget_backend: openai_chat. Both entry points only resolved a high-level
--backend label when a role was missing, so for any run using the shipped
defaults the label was silently discarded and the run executed on openai_chat.

  skillopt-train --config configs/searchqa/default.yaml --backend cursor
  ...
  [model config] backend=cursor_exec  optimizer=... (openai_chat)  target=... (openai_chat)

train.py guarded on "is either role unset?"; eval_only.py used
cfg.setdefault(), which is equally a no-op once the key exists. A role left at
the default openai_chat now counts as unset so the label wins, while a role the
operator explicitly pointed elsewhere still takes precedence.

The trainer's resolution moves to a module-level _resolve_role_backends() so it
is testable -- it previously sat inline inside Trainer.train().
Adds two backends. `copilot_chat` drives the Copilot CLI as a chat model and
can fill either role, so `--backend copilot` selects it for BOTH optimizer and
target -- the CLI carries its own sign-in, which makes that the only fully local
configuration: a complete train/eval loop with no cloud API key.
`copilot_exec` is the separate target-only execution harness, alongside the
existing codex/claude/cursor harnesses.

Verified end to end on SearchQA with no credentials configured: baseline eval,
rollout, reflect, aggregate, select, update and gate all execute against the
local CLI.

Safety: chat calls disable built-in MCP servers and custom instructions so the
model sees only the prompt SkillOpt sends, and never pass --allow-all-tools.
Unlike the other exec harnesses, `copilot_exec` does NOT grant unattended tool
use by default -- it requires an explicit `copilot_exec_allow_all_tools`
opt-in, because a file-edit rollout is the only case that needs it.

Two caveats worth knowing before use: the CLI is an agent rather than a
completions endpoint, so expect roughly 20-40 s per call; and it reports no
token counts, so usage totals are zero for these backends.

Depends on the --backend resolution fix: without it, --backend copilot is
discarded whenever the base config sets both role backends.
Copilot AI lite review requested due to automatic review settings August 4, 2026 13:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for running SkillOpt against the local GitHub Copilot CLI in two modes: copilot_chat (chat backend usable as optimizer and target for a fully local run) and copilot_exec (target-only exec harness). It also updates CLI/config wiring, documentation, and tests to cover the new backends and to ensure --backend correctly overrides role backends pinned by the base config.

Changes:

  • Introduce copilot_chat backend that drives the copilot CLI as a chat model (optimizer/target), with MCP servers and custom instructions disabled.
  • Add copilot_exec execution harness (target-only) with explicit opt-in gating for unattended tool use (--allow-all-tools).
  • Extend config/CLI/docs/tests and adjust role-backend resolution so --backend is not silently ignored when base defaults pin roles.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_role_backend_resolution.py New regression tests for role-backend resolution behavior.
tests/test_copilot_exec_backend.py Adds unit tests for Copilot backend normalization, configuration wiring, and exec/chat safety flags.
skillopt/model/copilot_backend.py Implements the Copilot CLI chat backend and JSONL parsing.
skillopt/model/common.py Registers Copilot backends in backend aliases/default-model map.
skillopt/model/codex_harness.py Adds run_copilot_exec harness + dispatch in run_target_exec.
skillopt/model/backend_config.py Adds Copilot-related env/config plumbing and backend whitelists.
skillopt/model/init.py Wires Copilot backends into set_backend / chat routing.
skillopt/engine/trainer.py Adds _resolve_role_backends and uses it during eval env construction.
skillopt/config.py Adds config-flattening keys for Copilot settings.
scripts/train.py Exposes Copilot backends and flags via legacy CLI args.
scripts/eval_only.py Fixes --backend overriding behavior and adds Copilot CLI/config wiring.
README.md Updates backend list to include Copilot backends.
docs/reference/config.md Documents Copilot backend availability and config keys.
docs/reference/api.md Documents Copilot backends in the public API reference tables.
docs/guide/configuration.md Adds Copilot CLI backend explanation, env vars, and safety notes.
configs/base/default.yaml Adds Copilot config fields and comments to base defaults.
CHANGELOG.md Notes new Copilot backends and their behavior/safety properties.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread skillopt/engine/trainer.py Outdated
Comment thread tests/test_role_backend_resolution.py
@lufen Christopher Haugen (lufen) changed the title Feat/copilot cli backend  feat(model): add the GitHub Copilot CLI as a backend Aug 4, 2026
…chat

The claude/claude_chat branch used 'x = x or default', but the base config
pins both roles to the truthy 'openai_chat', so --backend claude was still
silently ignored -- the very bug this resolver fixes for the other backends.
Switch it to the _ROLE_BACKEND_DEFAULTS check used elsewhere, and extend the
base-config regression parametrization to cover claude and claude_chat.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 16:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

skillopt/model/backend_config.py:115

  • This return statement is now long enough to be hard to read and is inconsistent with the multi-line style used for other backend sets in this module. Consider formatting it as a multi-line set literal to keep it readable and reduce churn when adding/removing backends.
def is_target_chat_backend() -> bool:
    return TARGET_BACKEND in {"openai_chat", "claude_chat", "qwen_chat", "minimax_chat", "openai_compatible", "copilot_chat"}

skillopt/model/init.py:114

  • get_backend_name() has a special-case for copilot_exec but not for the fully-local copilot_chat backend. When both roles are copilot_chat, this currently falls through to the generic "optimizer+target" string ("copilot_chat+copilot_chat"), which is inconsistent with the other unified backends and can confuse logs/telemetry that expect a canonical backend label.
    if optimizer == "openai_chat" and target == "copilot_exec":
        return "copilot_exec"
    if optimizer == "openai_compatible" and target == "openai_compatible":
        return "openai_compatible"
    return f"{optimizer}+{target}"

skillopt/model/backend_config.py:84

  • This target-backend whitelist is now a very long single line, unlike the optimizer whitelist above, and is likely to violate line-length/style checks. Wrapping it like the optimizer whitelist keeps formatting consistent and easier to edit when adding more backends.

This issue also appears on line 114 of the same file.

    TARGET_BACKEND = normalize_backend_name(backend or "openai_chat")
    if TARGET_BACKEND not in {"openai_chat", "claude_chat", "qwen_chat", "minimax_chat", "openai_compatible", "copilot_chat", "codex_exec", "claude_code_exec", "cursor_exec", "copilot_exec"}:

Addresses re-review: get_backend_name() special-cased copilot_exec and the
other unified chat backends (claude_chat, qwen_chat) but not copilot_chat, so
a fully-local run reported the generic 'copilot_chat+copilot_chat'. Return the
canonical 'copilot_chat' for both-role copilot, and assert it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 5, 2026 06:10
@lufen

Copy link
Copy Markdown
Contributor Author

Follow-up in 95d5643 addresses the re-review's suppressed suggestion: get_backend_name() now returns the canonical copilot_chat for a both-role fully-local run, consistent with claude_chat/qwen_chat, instead of the generic copilot_chat+copilot_chat. (The long set-literal style note is cosmetic; ruff is clean, so left as-is.) The inline --backend claude override bug was fixed in cb551d9.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

skillopt/model/backend_config.py:35

  • COPILOT_EXEC_ALLOW_ALL_TOOLS is read directly from the environment, but get_copilot_exec_config() only accepts values '0' or '1'. If a user sets the env var to a common boolean string like 'true'/'false', SkillOpt will raise ValueError at runtime. Consider normalizing the env var on load the same way other boolean-ish flags are parsed so 'true' becomes '1' and 'false' becomes '0'.
COPILOT_EXEC_ALLOW_ALL_TOOLS = os.environ.get("COPILOT_EXEC_ALLOW_ALL_TOOLS", "0")

Re-review catch: the module-level read took the env var raw, so setting
COPILOT_EXEC_ALLOW_ALL_TOOLS=true/false (without calling configure_copilot_exec)
made get_copilot_exec_config() raise ValueError. Normalize it through the
existing _parse_bool helper to '0'/'1' (unknown values fall back to the safe
'0'), matching the other boolean-ish exec flags. Regression added.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 5, 2026 06:31
@lufen

Copy link
Copy Markdown
Contributor Author

Second re-review follow-up in 9ec24fc: COPILOT_EXEC_ALLOW_ALL_TOOLS was read raw at module load, so setting it to true/false in the environment (without calling configure_copilot_exec) made get_copilot_exec_config() raise ValueError. It's now normalized through the existing _parse_bool helper to 0/1 (unknown values fall back to the safe 0), consistent with the other boolean-ish exec flags. Regression added; suites green.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

skillopt/model/codex_harness.py:1407

  • When the Copilot CLI emits no assistant messages (empty/invalid JSONL), the loop records each attempt’s stdout/stderr in all_raw, but the final RuntimeError(last_error) drops that context. Unlike other exec harnesses (e.g., cursor/codex) this makes a “no response” failure hard to debug because callers get neither persisted artifacts nor any CLI output.
    combined = "\n\n".join(all_raw)
    raise RuntimeError(last_error)

Re-review catch: the final failure path computed 'combined' from all_raw and
then discarded it, raising a bare 'Copilot CLI returned no response'. Unlike
the cursor/codex harnesses, copilot_exec persists no artifacts, so an empty or
invalid JSONL stream left the caller with nothing to debug. Append a bounded
(4000-char) tail of the captured stdout/stderr to the error. Regression added.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 5, 2026 06:52
@lufen

Copy link
Copy Markdown
Contributor Author

Third re-review follow-up in bd8e786: the final failure path in run_copilot_exec computed combined from all_raw and then discarded it, raising a bare Copilot CLI returned no response. Unlike the cursor/codex harnesses, copilot_exec persists no artifacts, so an empty or invalid JSONL stream left the caller with no output to debug. The error now carries a bounded (4000-char) tail of the captured stdout/stderr, with a regression covering it.

Suites green (509 passed).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

skillopt/model/common.py:28

  • default_model_for_backend() returns an empty string for copilot_chat/copilot_exec. This propagates into the CLI entrypoints (e.g. scripts/eval_only.py uses default_model_for_backend(backend) as a fallback for optimizer/target deployments), and can leave deployments empty (notably for --backend copilot_exec, where the optimizer is still openai_chat). Returning a real default here (or omitting these keys to fall back to the Azure/OpenAI default) avoids accidentally configuring an empty deployment.
    "copilot_exec": "",
    "copilot_chat": "",

skillopt/model/codex_harness.py:1384

  • On subprocess.TimeoutExpired, this captures only exc.stdout and drops exc.stderr. The other exec harnesses include stderr in their raw capture, and it’s important for debugging when Copilot emits errors to stderr before timing out.
        except subprocess.TimeoutExpired as exc:
            raw = exc.stdout or ""
            if isinstance(raw, bytes):
                raw = raw.decode("utf-8", "replace")
            all_raw.append(f"===== COPILOT CLI ATTEMPT {attempt + 1} =====\n{raw}")
            raise

…out stderr

Two re-review catches:

- _BACKEND_DEFAULT_MODELS mapped copilot_chat/copilot_exec to the empty string.
  That table also feeds the shared Azure deployment fallback in the entry
  points -- cfg.get("optimizer_model", default_model_for_backend(backend)) --
  and the shipped base config sets no optimizer_model/target_model, so the
  fallback is reached: --backend copilot_exec configured an EMPTY optimizer
  deployment even though that role is still a real openai_chat model. Drop the
  entries so they fall back to the Azure default; the CLI's own model continues
  to come from copilot_chat_optimizer_model / copilot_chat_target_model.
- run_copilot_exec dropped exc.stderr on TimeoutExpired; the codex/cursor
  harnesses all capture it, and Copilot can report the cause there before
  timing out. Capture it the same way.

Regressions added for both.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 5, 2026 07:01
@lufen

Copy link
Copy Markdown
Contributor Author

Fourth re-review follow-up in 9f43455 — both suggestions were correct, and the first turned out to be a real bug:

Empty optimizer deployment. _BACKEND_DEFAULT_MODELS mapped copilot_chat/copilot_exec to "". That table also feeds the shared deployment fallback in the entry points — cfg.get("optimizer_model", default_model_for_backend(backend)) — and the shipped configs/_base_/default.yaml sets no optimizer_model/target_model keys, so the fallback really is reached. That meant --backend copilot_exec configured an empty optimizer deployment, even though that role is still a real openai_chat model. Dropped the two entries so they fall back to the Azure default; the CLI's own model is unaffected because it comes from copilot_chat_optimizer_model / copilot_chat_target_model.

Timeout stderr. run_copilot_exec captured only exc.stdout on TimeoutExpired. The codex and cursor harnesses both fold stderr into their raw capture, and Copilot can report the cause there before timing out — now captured the same way.

Regressions added for both; suites green (510 passed).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

skillopt/model/codex_harness.py:1391

  • In run_copilot_exec, the TimeoutExpired handler spends effort normalizing/combining stdout+stderr and appending to all_raw, but then immediately re-raises. Since all_raw is never returned/persisted on this path, the combined raw is discarded and the comment about “captur(ing) it” is misleading. Either persist/propagate the combined output, or (simpler) remove the dead code and just re-raise like the other exec harnesses.
        except subprocess.TimeoutExpired as exc:
            raw = exc.stdout or ""
            if isinstance(raw, bytes):
                raw = raw.decode("utf-8", "replace")
            # Copilot can report the cause on stderr before timing out; the
            # other exec harnesses capture it, so this must too.
            err = exc.stderr or ""

@lufen

Copy link
Copy Markdown
Contributor Author

Follow-up in ae5aca5 addresses the suppressed alias-resolution finding. _resolve_role_backends() now canonicalizes its high-level backend argument with normalize_backend_name() before resolving each role, so aliases such as copilot_cli, github_copilot, anthropic, cursor_agent, compat, and openai-compatible no longer fall through to openai_chat. The same change also closes the canonical minimax_chat and openai_compatible mapping gaps exposed by normalizing first.

Regression coverage now exercises every affected alias family; the focused Copilot/backend-resolution suites pass (76 passed).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_copilot_exec_backend.py:464

  • These assertions are brittle because they depend on the scripts using double-quoted string literals; a harmless formatting change (e.g., switching to single quotes) would fail the test even though the CLI still exposes the backends. Prefer checking for the backend names without including quote characters.
    assert '"copilot_chat"' in text
    assert '"copilot_exec"' in text

Check backend names independently of the source file's string-literal style.

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

Copilot-Session: 0e8472e4-56ad-4daf-80b4-1c0ed0258133
Copilot AI review requested due to automatic review settings August 6, 2026 08:09
@lufen

Copy link
Copy Markdown
Contributor Author

Follow-up in 4b4d9c5 fixes the suppressed test-quality comment: the CLI exposure test now checks copilot_chat and copilot_exec independently of Python string-literal quote style. Focused suite: 38 passed; Ruff clean.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

configs/base/default.yaml:34

  • The comment for copilot_exec_allow_all_tools says “blank uses …”, but the config value is null (not an empty string) and the implementation treats None/null as “don’t override the env var”. Updating the comment avoids confusion for operators editing YAML.
  copilot_exec_allow_all_tools: null   # blank uses COPILOT_EXEC_ALLOW_ALL_TOOLS (default off);
                                       # copilot_exec only, required for file-edit rollouts

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
Copilot AI review requested due to automatic review settings August 6, 2026 08:22
@lufen

Copy link
Copy Markdown
Contributor Author

Consolidated follow-up in 5497a31 fixes the null wording and proactively addresses the same review patterns across the full PR diff:

  • corrected the YAML comment to say null preserves the environment setting;
  • corrected stale changelog cloud/local and safety claims;
  • documented that copilot_chat does not support caller-supplied/structured tool calls;
  • replaced duplicated broad-exception JSONL parsers with one typed, shape-safe parser;
  • centralized stripping of inherited COPILOT_ALLOW_ALL for both chat and exec subprocesses;
  • normalized the legacy set_backend() path through the canonical alias map and removed unreachable post-normalization branches;
  • replaced source-text CLI tests with real --help behavior plus AST call checks;
  • corrected the sign-in command and nonexistent example config path, and expanded coverage to every Copilot CLI flag.

Validation: focused Copilot/role suites 76 passed; broader Copilot/Codex/Cursor backend suites 104 passed; Ruff clean for changed core/test files.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/eval_only.py:346

  • In eval_only.py, _set_role() overwrites role backends even when they were explicitly set in the config file (not just via CLI args/--cfg-options). That contradicts the intended behavior (explicit role backends should outrank the high-level --backend label) and can unexpectedly clobber non-default optimizer_backend/target_backend values from a custom YAML config. Only override roles when they are still at the shipped defaults (None/""/"openai_chat").
        def _set_role(key: str, value: str) -> None:
            """Assign a role backend unless the operator named one explicitly.

            ``setdefault`` was a no-op here: configs/_base_/default.yaml always
            sets both roles, so an explicit --backend was silently ignored.
            """
            if not _has_model_override(f"model.{key}", key):
                cfg[key] = value

Only apply a high-level backend label to inherited default role values. Preserve non-default YAML roles and explicit CLI role overrides, with direct regression coverage.

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

Copilot-Session: 0e8472e4-56ad-4daf-80b4-1c0ed0258133
Copilot AI review requested due to automatic review settings August 6, 2026 08:31
@lufen

Copy link
Copy Markdown
Contributor Author

Fixed in 8208cd4. Eval-only now applies a high-level backend mapping only when the current role is still one of the shipped defaults (None, "", or openai_chat) and the role was not explicitly overridden on the CLI. Non-default role values from custom YAML are preserved, as are explicit CLI role overrides even when their value is openai_chat.

Added direct regressions for inherited defaults, custom-YAML roles, and explicit CLI roles. Focused suites: 81 passed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

skillopt/model/copilot_backend.py:60

  • _messages_to_prompt() silently drops non-text multipart message parts (e.g. {"type": "image_url" ...}), which will produce incorrect prompts for environments like DocVQA that send images via chat_target_messages. This backend should fail fast (or explicitly encode/mark unsupported parts) rather than ignoring them.
        content = message.get("content")
        if isinstance(content, list):  # OpenAI multi-part content
            content = "\n".join(
                str(part.get("text", "")) for part in content if isinstance(part, dict) and part.get("type") == "text"
            )

skillopt/model/common.py:31

  • The comment says copilot_chat / copilot_exec are "deliberately absent" from _BACKEND_DEFAULT_MODELS, but default_model_for_backend() still returns the azure_openai fallback (currently gpt-4o) for these backends. As written, the comment is misleading about the actual behavior and why leaving them out is safe.
    # copilot_chat / copilot_exec are deliberately absent: the CLI picks its own
    # model (configured via copilot_chat_optimizer_model / _target_model), and
    # this table also feeds the shared Azure deployment fallback. Mapping them
    # to "" left `--backend copilot_exec` with an EMPTY optimizer deployment,
    # even though that role is still a real openai_chat model.

Fail fast for non-text multipart content and clarify that Copilot backends intentionally use the shared deployment fallback without selecting the CLI model.

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

Copilot-Session: 0e8472e4-56ad-4daf-80b4-1c0ed0258133
Resolve Copilot roles before model defaults and omit inherited OpenAI deployment sentinels for copilot_exec. Preserve explicit target model selections and add entry-point regression coverage.

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

Copilot-Session: 0e8472e4-56ad-4daf-80b4-1c0ed0258133
Copilot AI review requested due to automatic review settings August 6, 2026 08:55
@lufen

Copy link
Copy Markdown
Contributor Author

Addressed both suppressed findings in 32c4563: copilot_chat now fails fast for any non-text multipart content (including image_url) before invoking the CLI, and the default-model comment now accurately explains that Copilot backends intentionally receive the shared Azure deployment fallback while CLI model selection remains separate.

I also ran a full local review of upstream/main...HEAD across all 17 changed files. It found one additional high-confidence issue, fixed in 0ffd2cf: copilot_exec inherited the shipped target_model: gpt-5.5 and passed it to Copilot CLI as --model gpt-5.5. Train and eval now resolve Copilot roles before model defaults and omit inherited OpenAI sentinels, while preserving an explicitly selected Copilot model ID.

Regression suites: 84 passed. The full local diff review found no other high-confidence issues after that fix.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

configs/base/default.yaml:37

  • copilot_chat_timeout is set to 0, but configure_copilot_chat() rejects non-positive timeouts. The scripts avoid this by passing cfg.get('copilot_chat_timeout') or None, but keeping an invalid literal in the shipped base config is brittle (e.g., callers that pass the config value through directly will get a ValueError). Prefer using null to mean “no override / use env default”, consistent with copilot_exec_allow_all_tools: null.
  copilot_chat_timeout: 0            # 0 uses COPILOT_CHAT_TIMEOUT or the built-in default

Keep the shipped timeout valid for direct config pass-through and preserve environment or built-in defaults when no override is requested.

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

Copilot-Session: 0e8472e4-56ad-4daf-80b4-1c0ed0258133
Copilot AI review requested due to automatic review settings August 6, 2026 09:01
@lufen

Copy link
Copy Markdown
Contributor Author

Fixed in 549390e: the shipped copilot_chat_timeout default is now null, so direct config pass-through preserves COPILOT_CHAT_TIMEOUT/the built-in default instead of supplying an invalid 0. Regression coverage verifies both nullable Copilot defaults are no-ops and preserve prior environment-derived settings. Focused suites: 84 passed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

@Yif-Yang
Yifan Yang (Yif-Yang) merged commit d4f1a53 into microsoft:main Aug 6, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants