Skip to content

fix(copilot): expose only account-available models - #4698

Open
hgaol wants to merge 2 commits into
apache:mainfrom
hgaol:fix/github-copilot-available-models
Open

fix(copilot): expose only account-available models#4698
hgaol wants to merge 2 commits into
apache:mainfrom
hgaol:fix/github-copilot-available-models

Conversation

@hgaol

@hgaol hgaol commented Sep 4, 2026

Copy link
Copy Markdown
Member

Title

fix(copilot): expose only account-available models

Description

Summary

GitHub Copilot model discovery could expose models the connected account could not use.

This fixes both sources:

  1. Provider response filtering

    • Policy-free models remain valid.
    • A present policy must be exactly enabled.
    • disabled, unconfigured, unknown, and malformed policies are excluded.
    • Existing picker, tool-call, and supported-wire requirements remain unchanged.
  2. Catalog persistence

    • Copilot's filtered /models response is treated as the account's authoritative usable catalog.
    • The first account-scoped discovery replaces bootstrap fallback IDs with the available account models.
    • Later refreshes remove withdrawn IDs while preserving still-available user selections.
    • Newly introduced models remain opt-in after the initial discovery.
    • If an unavailable default must be replaced, reconciliation selects an available model.

The generic reconciliation behavior for other providers is unchanged: their model lists may be partial, so omitted user-selected IDs remain preserved.

Fixes #4697

Root cause

toGitHubCopilotModelInfo rejected only:

policy?.state === 'disabled'

That admitted unconfigured models even though Maka cannot accept their policy terms.

Separately, reconcileConnectionAfterModelFetch intentionally preserved every previously enabled model. Interactive Copilot connections start with fallback IDs, so models absent from the account response were added back into the projected catalog.

Verification

The regressions fail without this change and pass with it:

  • Provider-contract fixture with:
    • policy-free model;
    • enabled model;
    • disabled model;
    • unconfigured model;
    • hidden-picker model;
    • tool-incompatible model;
    • unsupported endpoint.
  • Core authoritative-reconciliation tests:
    • initial fallback replacement;
    • withdrawn model removal;
    • still-available user choice preservation;
    • newly introduced model remains opt-in.
  • Runtime Policy integration test proving unavailable bootstrap IDs are removed from persisted Copilot state.

Checks run:

  • Provider conformance matrix: 126 passed
  • @maka/core: 782 passed
  • @maka/storage: 1107 tests, 0 failures
  • @maka/runtime: 3176 tests, 0 failures
  • Full workspace build: passed
  • Full workspace typecheck: passed
  • Lint: passed
  • Format check: passed
  • ASF source-header check: passed

The focused regressions and full typecheck/lint/format were rerun after merging the latest main.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: pi with an OpenAI model assisted diagnosis, primary-source comparison against GitHub's Copilot client, implementation, test creation, and verification. The human contributor remains responsible for the change.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — GitHub Copilot model lists now omit models that are unavailable to the connected account.
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 4, 2026
@hgaol
hgaol force-pushed the fix/github-copilot-available-models branch 2 times, most recently from ae320e1 to dba7211 Compare September 4, 2026 14:22

@Astro-Han Astro-Han 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.

Reviewed exact head dba7211ec39a99c12481a77a497ed6f9d68b8a29. I found no P0–P3 issues.

What the diff actually does (7 files, +139/−17, two commits):

  1. packages/runtime/src/model-fetcher.ts — the Copilot discovery filter now admits a model only when policy is absent or exactly { state: 'enabled' } (isGitHubCopilotModelPolicyEnabled). unconfigured, disabled, malformed, and unknown states are excluded, failing closed. The existing picker, tool-call, and supported-endpoint requirements are unchanged.
  2. packages/core/src/llm-connections.tsreconcileConnectionAfterModelFetch gains an authoritative option: the first account-scoped fetch replaces bootstrap fallback ids with the fetched catalog, and later refreshes keep only user-enabled ids still present in the catalog, so newly introduced models stay opt-in.
  3. packages/storage/src/runtime-policy/connection-catalog-document.ts — the single production reconcile call site sets authoritative only for github-copilot, so generic providers keep the existing preserve-everything behavior.
  4. scripts/release-cli-package.mjs — the production dependency audit retries once on transient 5xx and transport errors; real vulnerability reports still fail closed.

What I checked:

  • I read the full diff plus the surrounding reconcile logic, including alias mapping (supersededModelId) and the hasModelInventory derivation (previous.models.length > 0; the catalog codec forbids a non-empty models array before discovery, so the first Copilot fetch always takes the replace-bootstrap branch).
  • Red-green per layer: with each changed file reverted to the base commit, the new core reconciliation test, the provider conformance matrix, and the storage runtime-policy integration test fail; with the PR applied all three pass. Local results on this head: core 14/14, provider contract matrix 126/126, storage runtime-policy stores 69/69.
  • Hosted checks on this exact head are green: test, four platform addon builds, immutable tarball, four Validate-installed-CLI jobs, and Qualify released State Roots. The Validate-installed-CLI Eval job shows as skipped in a non-required matrix cell.
  • No database schema change; the reconcile option is call-time only and never persisted.

Two observations, neither blocking:

  • When every user-enabled model has been withdrawn but the account catalog is still non-empty, the reconciler force-enables the first catalog entry and moves the default to it (packages/core/src/llm-connections.ts:507, branch at :498). That sits in tension with the adjacent comment that newly introduced models stay opt-in (:502); the counter-argument is that it keeps the connection usable instead of leaving zero enabled models. That trade-off deserves a deliberate product decision at some point.
  • An account whose entire catalog is policy-gated now fetches zero models, so a refresh (and onboarding verify) fails with the generic invalid_response error class (packages/runtime-host/src/server/connection-effect-coordinator.ts:158, :195). Failing closed is the right direction; the error just does not tell the user it is a policy or entitlement issue.

Housekeeping: the PR body declares a substantive generative-tool contribution, but neither commit carries a Generated-by: trailer. With the repository's squash-merge message setting, such a trailer would be lost at merge time regardless, which is worth noting for whoever merges.

This is not an approval; the merge decision stays with the human reviewers.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@hgaol
hgaol force-pushed the fix/github-copilot-available-models branch from dba7211 to 22d1e87 Compare September 5, 2026 06:35
GitHub's model policy has three historical states: enabled, disabled, and
unconfigured. Maka cannot accept model policy terms, so exclude every present
policy that is not enabled while continuing to admit policy-free current
responses.

Treat the filtered Copilot /models response as an authoritative account
catalog during persistence. The first fetch replaces bootstrap fallback IDs;
later refreshes remove withdrawn IDs without automatically opting the user
into newly introduced models.

Cover the mixed provider payload, authoritative reconciliation, and real
Runtime Policy model-fetch commit.

Generated-by: gpt-5.6-sol
The required CLI packaging step can receive a five-minute HTTP 503 from npm's
advisory endpoint. Retry that existing audit command once for 5xx and common
transport failures; vulnerability reports and all deterministic failures remain
fail-closed.

Keep the fix local to release-cli-package.mjs: no package scripts, dependencies,
or workflow changes.

Generated-by: gpt-5.6-sol
@hgaol
hgaol force-pushed the fix/github-copilot-available-models branch from 22d1e87 to 5f0997f Compare September 5, 2026 06:38
@hgaol

hgaol commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

Reviewed exact head dba7211ec39a99c12481a77a497ed6f9d68b8a29. I found no P0–P3 issues.

What the diff actually does (7 files, +139/−17, two commits):

  1. packages/runtime/src/model-fetcher.ts — the Copilot discovery filter now admits a model only when policy is absent or exactly { state: 'enabled' } (isGitHubCopilotModelPolicyEnabled). unconfigured, disabled, malformed, and unknown states are excluded, failing closed. The existing picker, tool-call, and supported-endpoint requirements are unchanged.
  2. packages/core/src/llm-connections.tsreconcileConnectionAfterModelFetch gains an authoritative option: the first account-scoped fetch replaces bootstrap fallback ids with the fetched catalog, and later refreshes keep only user-enabled ids still present in the catalog, so newly introduced models stay opt-in.
  3. packages/storage/src/runtime-policy/connection-catalog-document.ts — the single production reconcile call site sets authoritative only for github-copilot, so generic providers keep the existing preserve-everything behavior.
  4. scripts/release-cli-package.mjs — the production dependency audit retries once on transient 5xx and transport errors; real vulnerability reports still fail closed.

What I checked:

  • I read the full diff plus the surrounding reconcile logic, including alias mapping (supersededModelId) and the hasModelInventory derivation (previous.models.length > 0; the catalog codec forbids a non-empty models array before discovery, so the first Copilot fetch always takes the replace-bootstrap branch).
  • Red-green per layer: with each changed file reverted to the base commit, the new core reconciliation test, the provider conformance matrix, and the storage runtime-policy integration test fail; with the PR applied all three pass. Local results on this head: core 14/14, provider contract matrix 126/126, storage runtime-policy stores 69/69.
  • Hosted checks on this exact head are green: test, four platform addon builds, immutable tarball, four Validate-installed-CLI jobs, and Qualify released State Roots. The Validate-installed-CLI Eval job shows as skipped in a non-required matrix cell.
  • No database schema change; the reconcile option is call-time only and never persisted.

Two observations, neither blocking:

  • When every user-enabled model has been withdrawn but the account catalog is still non-empty, the reconciler force-enables the first catalog entry and moves the default to it (packages/core/src/llm-connections.ts:507, branch at :498). That sits in tension with the adjacent comment that newly introduced models stay opt-in (:502); the counter-argument is that it keeps the connection usable instead of leaving zero enabled models. That trade-off deserves a deliberate product decision at some point.
  • An account whose entire catalog is policy-gated now fetches zero models, so a refresh (and onboarding verify) fails with the generic invalid_response error class (packages/runtime-host/src/server/connection-effect-coordinator.ts:158, :195). Failing closed is the right direction; the error just does not tell the user it is a policy or entitlement issue.

Housekeeping: the PR body declares a substantive generative-tool contribution, but neither commit carries a Generated-by: trailer. With the repository's squash-merge message setting, such a trailer would be lost at merge time regardless, which is worth noting for whoever merges.

This is not an approval; the merge decision stays with the human reviewers.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

Resolved and added generated-by

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(copilot): model catalog includes unavailable and unconfigured models

2 participants