feat(copilot): synthesize GitHub Copilot models dynamically from CAPI catalog - #1583
Conversation
Code Review —
|
# Conflicts: # packages/coding-agent/CHANGELOG.md # packages/coding-agent/docs/models.md # packages/coding-agent/docs/providers.md # packages/coding-agent/src/core/copilot-model-catalog.ts # packages/coding-agent/src/core/model-registry-builtins.ts # packages/coding-agent/test/copilot-model-catalog.test.ts # packages/coding-agent/test/model-registry-context-window.suite.ts
Code Review — PR #1583: feat(copilot): add claude-sonnet-5 and mai-code-flash-1 builtin modelsThanks for this — the pivot from a hardcoded model list to dynamic synthesis from the live CAPI catalog is the right architectural call. It generalizes cleanly (new Copilot models appear without a fork edit), the gating logic is careful, and the test coverage is strong. A few things worth addressing before merge, none blocking on their own. 🔴 PR description no longer matches the implementationThe description documents an The actual mechanism is 🟠 Domain docs stripped to fit under the 500-line gate
Consider splitting the file (e.g. extract the disk-cache read/write/sanitize block into a 🟡 cost: ZERO_COST on every synthesized model
🟡 maxTokens fallback can land on contextWindowmaxTokens: entry.maxTokens ?? entry.limits?.maxOutputTokens ?? entry.maxInputTokens ?? entry.contextWindow,The final fallback uses an input budget ( 🟢 Minor / nits
✅ Strengths
Overall: solid, well-tested work. The must-fix items are (1) syncing the PR description to the real implementation and (2) restoring the stripped catalog docs (ideally via a file split). The cost/maxTokens items are worth a quick confirm. Reviewed by Claude (Opus 4.8). |
Assistant-model: GPT-5.5
Code Review — dynamic GitHub Copilot model synthesisNice refactor. Replacing a hardcoded two-model allowlist with a general, catalog-driven synthesis mechanism is the right call, and the gating ( A few things worth a look: Potential bug —
|
Assistant-model: GPT-5.5
Review: dynamic GitHub Copilot model synthesisNice work — this is a well-structured change. The catalog-driven approach (zero hardcoded model ids), the disk-cache schema bump to Bugs / correctness1. maxTokens: entry.maxTokens ?? entry.limits?.maxOutputTokens ?? entry.maxInputTokens ?? entry.contextWindow,When CAPI advertises neither 2. this._emit({ type: "model_changed", model: replay.model, previousModel, source: "restore" });whereas Maintainability3. 4. Dropped JSDoc on nontrivial logic. The diff removes the detailed rationale comments (the Minor
SecurityNo concerns — no new secrets/logging, catalog fetch reuses the existing auth-gated token flow, cache is host-scoped, and cached fields are re-validated on read ( Overall a clean, well-tested change. #1 (output-cap fallback) is the only item I'd consider blocking; the rest are polish. 🤖 Automated review by Claude Opus 4.8 |
The coding-agent suite runs under vitest, but the new catalog-refresh test imported afterEach/test from bun:test, failing suite collection on both linux-x64 and windows-x64 CI jobs. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Review: Dynamic GitHub Copilot model synthesis from CAPI catalogOverall this is a clean, well-structured change that meaningfully improves on the "hardcode two model ids" alternative. The catalog-driven synthesis is nicely factored ( A few things worth a look before merge: 1. Session refresh persists a clamped thinking level into global settings (medium)
So when the live catalog restricts a Copilot model to fewer levels than the fallback offered, the automatic refresh silently rewrites the user's global thinking preference. Concretely: user launches with Note the asymmetry: this refresh path deliberately avoids persisting the model change (unlike the explicit 2. `model_changed` now fires on unrelated `unregisterProvider` calls (low)
3. Policy gating only excludes exactly `"disabled"` (low)
4. `copilot-model-catalog.ts` is now 498 lines (nit)Right up against the 500-line 5. Removed field-level JSDoc (nit)The PR trims the per-field doc comments on Minor
None of these block the core mechanism, which is solid. #1 is the one I'd most want addressed since it silently mutates a user preference across providers. |
Summary
Replaces the plan to hardcode
claude-sonnet-5andmai-code-flash-1as static built-in Copilot models with a general mechanism:github-copilotmodels are now synthesized dynamically from GitHub's live CAPI/modelscatalog, so any picker-enabled, non-disabled, plain-id chat model (including these two, and any future additions) is automatically exposed as agithub-copilot/*model without a code change or hardcoded metadata list. The active session's model and thinking level are also kept live-synced when the catalog loads or refreshes mid-session.Key Changes
src/core/copilot-model-synthesis.ts, new):synthesizeCopilotCatalogModels()walks the live CAPI catalog and builds aModel<Api>for every entry that ismodel_picker_enabled, not policy-disabled,type: "chat", has a plain (non-namespaced) id, and maps to a supported endpoint (/v1/messages→anthropic-messages,/responses→openai-responses,/chat/completions→openai-completions, in that preference order). Reasoning/thinking metadata, vision input, cost (zeroed, billed by Copilot), and context-window options are derived from catalogsupports/limitsrather than hand-written per model.copilotThinkingLevelMapFor()maps CAPI's advertisedreasoningEffortLevels(oradaptiveThinkingas a fallback) onto Atomic's thinking-level map, gating anofflevel by API and adaptive-thinking support. Namespaced enterprise deployment ids (e.g.org/deployment/model) are skipped, and ids already present in the upstreampi-aibuilt-ins always win over a synthesized entry.src/core/copilot-model-catalog.ts):CopilotModelContextnow also carriesdisplayName,vendor,supportedEndpoints,supports(adaptive thinking, reasoning effort/levels, vision, tool calls, etc.),limits,modelPickerEnabled,policyState, andtype, all parsed from CAPI's/modelsresponse and round-tripped through the on-disk cache (cache schema bumped to version5).src/core/model-registry-builtins.ts):loadBuiltInModels()now callswithDynamicGitHubCopilotModels(), which dedupes against the upstream catalog by id and appends any freshly synthesized Copilot models using a connection template (baseUrl/headers) derived from an existing siblinggithub-copilotmodel. A newwithCopilotThinkingLevelMap()also reapplies CAPI-derived reasoning-effort gating to existing (non-synthesized) Copilot models so their thinking levels stay in sync with the live catalog.src/core/agent-session-extension-bindings.ts,src/core/agent-session-methods.ts,src/modes/interactive/interactive-model-routing.ts): adds a publicAgentSession.refreshCurrentModelFromRegistry()(wrapping the existing internal_refreshCurrentModelFromRegistry) that re-resolves the active model against the current registry, clamps the active thinking level to whatever the refreshed model now supports viasetThinkingLevel(), and emits amodel_changedevent (source: "restore") alongside anycontext_window_changed/thinking_level_changedevents.loadCopilotModelCatalognow calls this after applying a freshly fetched or cached Copilot catalog, so a session that started on a fallback/stale Copilot model definition picks up the live catalog's metadata (e.g. corrected thinking levels) without a restart — while leaving the active model untouched if the registry still can't resolve it.test/copilot-model-synthesis.test.ts(endpoint→API mapping, capability/thinking-level derivation, gating of non-picker/disabled/non-chat/namespaced/duplicate ids, deterministic endpoint preference) and newtest/agent-session-copilot-catalog-refresh.test.ts(session adopts refreshed catalog metadata and clamps stale thinking levels, leaves the session untouched when the registry can't resolve the active model, andloadCopilotModelCatalogtriggers the session refresh after applying a cached catalog), plus updatedtest/copilot-model-catalog.test.tsandtest/model-registry-context-window.suite.tscovering the richer catalog metadata (includingclaude-sonnet-5andmai-code-1-flash-pickerfixtures) end-to-end through CAPI parsing, disk-cache round-tripping, and the registry overlay.docs/models.md,docs/providers.md,docs/rpc.md,docs/sdk.md, anddocs/settings.mdupdated to describe dynamic catalog-driven model population (vs. the prior fixed allowlist) and to useclaude-sonnet-5/mai-code-1-flash-pickeras examples.[Unreleased] → ### Addeddocuments the dynamic population behavior and catalog-driven thinking-level gating.Notes
@earendil-works/pi-ai; no version bumps.claude-sonnet-5andmai-code-flash-1) is reflected automatically on the next catalog fetch — no follow-up PR needed once GitHub's CAPI lists them.bun run typecheck,bun run lint,bun run check:file-length, andAGENT=1 bun run test:unitall green.AI-assisted (Claude Fable 5 via Atomic goal workflow).