fix: resolve COPILOT_MODEL aliases at runtime before spawning Copilot CLI#44975
Conversation
… CLI (#44260) - Add resolve_model_alias.cjs with fallback resolution algorithm - Add resolve_model_alias.test.cjs with 7 passing tests - In copilot_harness.cjs: add AWF_CONFIG_PATH, loadAwfConfigData(), applyCopilotModelAliasResolution(), and call it in main() after AWF reflect data fetch so aliases like 'small' resolve to concrete model ids before COPILOT_MODEL is consumed by the CLI Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Remove duplicate aliasMap check in resolveAliasEntry() - Use __dirname from import.meta.url for robust path resolution in tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Copilot CLI failures by resolving engine.model aliases (e.g. small, sonnet, opus?effort=high) into concrete model IDs at runtime before spawning the Copilot CLI, ensuring COPILOT_MODEL is always a CLI-accepted value.
Changes:
- Adds a new runtime alias resolution module that loads the alias map, builds a catalog from AWF
/reflect, resolves multi-hop alias chains, and normalizes results for the Copilot CLI. - Wires alias resolution into
copilot_harness.cjsafter AWF reflect data is fetched soprocess.env.COPILOT_MODELis rewritten before the CLI starts. - Adds Vitest unit tests covering glob matching, version selection, alias chain resolution, CLI normalization, and reflect-catalog building.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/resolve_model_alias.cjs | Implements the runtime alias→concrete resolution algorithm and CLI normalization. |
| actions/setup/js/resolve_model_alias.test.cjs | Adds unit tests for the alias resolution and catalog-building logic. |
| actions/setup/js/copilot_harness.cjs | Applies alias resolution to COPILOT_MODEL after AWF reflect data is available, before spawning Copilot CLI. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44975 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics (7 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three items.
📋 Key Themes & Highlights
Issues
- Silent error swallowing (
loadAwfConfigData): JSON parse errors on a malformed config file are swallowed with no log output, making debugging very hard. globMatchbare-wildcard gap: single-segment wildcard patterns (no/) are treated as exact equality. All current aliases useprovider/*...*so this is latent, but it is a correctness gap.- Missing
opusplantest: the query-param inheritance chain (opusplan → opus?effort=high → copilot/*opus*) is the most complex path and is not covered.
Positive Highlights
- ✅ Multi-hop alias resolution algorithm is well-structured and correctly handles circular reference detection
- ✅
mergeParamscorrectly gives caller params priority over entry params (spec §8.3 step 2b ii) - ✅
resolveConfiguredCopilotModelis a clean, deep module: simple entry point, rich behaviour inside - ✅ Good use of early-return no-ops when the model is already concrete
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 48.6 AIC · ⌖ 4.82 AIC · ⊞ 6.6K
Comment /matt to run again
There was a problem hiding this comment.
Review: resolve COPILOT_MODEL aliases at runtime
Good implementation overall — the multi-hop alias chain, query-param inheritance, and Copilot CLI normalization are all handled cleanly, and the harness integration is minimal and well-placed.
One correctness gap (non-blocking): globMatch silently degrades to exact equality for single-segment patterns that contain *. All current alias entries have provider prefixes so nothing breaks today, but bare-wildcard entries would fail silently. See inline comment for a suggested fix.
All other logic looks correct — circular-alias guard, catalog-empty early exit, and resolveAliasEntry fallback ordering are all sound.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 30.2 AIC · ⌖ 4.47 AIC · ⊞ 4.8K
There was a problem hiding this comment.
REQUEST_CHANGES — 4 correctness bugs + broken test suite
The logic is well-structured and the overall design is sound, but there are four issues that must be fixed before merge.
Blocking issues (in priority order)
-
Test file is unparseable (
resolve_model_alias.test.cjs): ES moduleimportsyntax in a.cjsfile is a SyntaxError — all 7 tests fail before any test runs. Rename to.test.mjsor convert torequire(). -
globMatchbare patterns never wildcard (resolve_model_alias.cjsline 145): single-segment patterns fall through to exact string equality, so any alias entry without a/(e.g.claude-*) silently fails to match anything. -
Date-stamped model version ordering is inverted (
resolve_model_alias.cjsline 175):extractVersionTupletakes the last numeric run, sogpt-4o-2024-08-06→[6]ranks lower thangpt-4o-2024-05-13→[13], selecting an older model as "latest". -
Diamond alias graphs fail with a false circular-reference error (
resolve_model_alias.cjsline 265): thevisitedSet is shared across all recursive branches; a second path converging on a previously-visited alias key is incorrectly rejected. Fix: clone the set when passing to each sibling entry.
Non-blocking: loadAwfConfigData should distinguish ENOENT (expected) from malformed JSON (unexpected, should log).
🔎 Code quality review by PR Code Quality Reviewer · 57.7 AIC · ⌖ 5.12 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/resolve_model_alias.test.cjs:1
All 7 tests are dead on arrival: .cjs files are always parsed as CommonJS by Node.js, and ES module import declarations are a SyntaxError in that context — the entire suite will fail to parse.
<details>
<summary>💡 Fix</summary>
Either rename to .test.mjs (or .test.js if the package is already "type": "module"), or replace all ESM imports with CommonJS require():
const { describe, it, expect } = require('vitest');
const fs = require('fs');
const path = require('path');
…
</details>
<details><summary>actions/setup/js/resolve_model_alias.cjs:145</summary>
**`globMatch` never wildcards a bare (non-scoped) pattern**: when `pattern` contains no `/`, the function falls through to a plain string equality check — wildcard characters in the pattern are never expanded.
<details>
<summary>💡 Detail + fix</summary>
Current code (line 144–145):
```js
if (patternParts.length === 1) {
return pattern.toLowerCase() === entry.toLowerCase(); // ← exact match, no glob
}An alias entry like "claude-*" or "*haiku*" will never match any catalog entry, …
actions/setup/js/resolve_model_alias.cjs:175
Date-stamped model names sort in wrong recency order: extractVersionTuple takes the last numeric run, so gpt-4o-2024-08-06 → [6] while gpt-4o-2024-05-13 → [13], making the May snapshot sort higher than the August one.
<details>
<summary>💡 Detail + fix</summary>
The regex /(\d+(?:\.\d+)*)/g collects runs of digits separated by .; the hyphens in a date like 2024-08-06 are not dots, so it produces four separate single-number matches: ['4','2024','08','06']. Taking the la…
actions/setup/js/resolve_model_alias.cjs:265
Diamond alias graphs silently fail: the shared visited Set causes a second convergent path to the same alias key to be misidentified as a circular reference and return null.
<details>
<summary>💡 Detail + fix</summary>
visited is allocated once per top-level resolveModelAlias call (line 256) and passed by reference through all recursion. Consider an alias map:
{ "small": ["mini"], "mini": ["haiku"], "tiny": ["mini"] }If small → mini succeeds, mini is added to …
actions/setup/js/copilot_harness.cjs:27
Malformed awf-config.json silently skips alias resolution with no diagnostic: the catch block returns null for any error, including JSON.SyntaxError, making a corrupted config file invisible to operators.
<details>
<summary>💡 Fix</summary>
function loadAwfConfigData() {
try {
return JSON.parse(fs.readFileSync(AWF_CONFIG_PATH, 'utf8'));
} catch (err) {
if (err.code !== 'ENOENT') {
// File exists but is unreadable or malformed — surface the problem
log(`…
</details>
|
@copilot please run the Unresolved review feedback to address:
|
…fix glob wildcards, log config errors, add tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All 7 unresolved review threads addressed in commit
All 214 JS tests pass locally. |
|
@copilot merge main and recompile |
There was a problem hiding this comment.
Smoke test review: inline comment flow exercised.
Warning
Firewall blocked 5 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · 120.7 AIC · ⌖ 4.96 AIC · ⊞ 19.8K
Comment /smoke-copilot to run again
Add label smoke to run again
|
@copilot fix must be applied to detection harness as well https://github.com/github/gh-aw/actions/runs/29186015811/job/86632400530#step:17:1 |
The threat detection job ran on a separate fresh runner with its own minimal awf-config.json that was missing the apiProxy.models alias map. When copilot_harness.cjs tried to resolve `COPILOT_MODEL=small`, the alias map was null so normalizeForCopilotCLI returned "small" unchanged, causing the CLI to reject it as unsupported. Fix: propagate data.ModelMappings into the threatDetectionData struct so BuildAWFConfigJSON includes the full model alias map in the detection step's awf-config.json, matching what the agent step already emits. Also: - Update outdated workflow_data.go comment (ModelMappings IS emitted) - Add regression test TestBuildDetectionEngineExecutionStepPropagatesModelMappings Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. Root cause: Fix: Added |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29187567992
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch merged with main (resolved 162 lock file conflicts by taking main's versions and recompiling). Local validation passes:
All 7 review threads remain resolved. CI needs to be re-triggered by a maintainer. |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29190625506
|
Branch is up to date with main (merged in the previous session). All 7 review threads are resolved. Local validation passes: |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29192100649
|
Branch is up to date with main, all 7 review threads remain resolved, and local validation passes: |
🤖 PR Triage
COPILOT_MODEL alias resolution fix. Large change (+706/-165). CI passed ✅. High-priority runtime bug — fast track.
|
All 7 review threads are already resolved ( |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29199165277
|
All 7 review threads remain resolved. Branch is up to date with main. CI smoke checks are green: runs |
engine.modelaliases (e.g.small,sonnet,copilot,opus?effort=high) compiled correctly but were passed as-is toCOPILOT_MODEL, which the Copilot CLI rejected — it only accepts concrete model ids.Changes
resolve_model_alias.cjs(new) — fallback resolution algorithm: loads alias map fromawf-config.json(apiProxy.models), builds a concrete catalog from AWF/reflectendpoint data, resolves multi-hop chains with query-param inheritance, and strips thecopilot/prefix for the CLI:copilot_harness.cjs— callsapplyCopilotModelAliasResolution()after AWF reflect data is fetched, rewritingprocess.env.COPILOT_MODELfrom alias to concrete id before the CLI is spawned. No-ops whenCOPILOT_MODELis already a concrete id.resolve_model_alias.test.cjs(new) — 7 unit tests covering glob matching, version selection, chain resolution, CLI normalization, and end-to-end flow.