Skip to content

fix(copilot): hide unavailable and unconfigured models - #4753

Open
seekskyworld wants to merge 3 commits into
apache:mainfrom
seekskyworld:fix/github-copilot-model-catalog-authoritative
Open

fix(copilot): hide unavailable and unconfigured models#4753
seekskyworld wants to merge 3 commits into
apache:mainfrom
seekskyworld:fix/github-copilot-model-catalog-authoritative

Conversation

@seekskyworld

Copy link
Copy Markdown
Contributor

Summary

Fixes #4697

GitHub Copilot model discovery now treats the account /models response as authoritative: policy-free and enabled models remain visible, disabled and unconfigured models are excluded, and refreshes drop bootstrap fallback ids that are no longer in the live catalog without auto-selecting a replacement.

Verification

  • npm exec -- tsx --test packages/runtime/src/__tests__/provider-contract-overrides.ts
  • npm exec -- tsx --test packages/storage/src/__tests__/runtime-policy-model-facts.test.ts

AI use

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

Tool(s) and scope:

  • Codex: implemented the Copilot model fetch/filter fix, added regression tests, and updated the local coordination/history notes.

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 — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 4, 2026

@hqhq1025 hqhq1025 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 db68937eaac43b109e788590a255ca8ae3e20a3e. The Copilot-only filtering is directionally correct, but two reachable cases still break the authoritative-catalog contract: an all-unavailable response cannot be committed, and a nullable policy crashes discovery. The required test check is also currently failing because the two changed test files are not Biome-formatted.

Local verification: clean npm 11.19 install, full build:test, Runtime 3,196 passed / 13 skipped, Storage 1,110 passed / 10 skipped, focused Runtime Host 29/29, focused Core 20/20, lint, ASF headers, diff check, and a clean patch-preserving merge onto current main (7743890729b26a7e95f653af8b93ccbe42ad480f). I did not call the live Copilot service; provider behavior was exercised through deterministic local HTTP fixtures.

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

Comment thread packages/storage/src/runtime-policy/connection-catalog-document.ts
Comment thread packages/runtime/src/model-fetcher.ts Outdated
@seekskyworld

Copy link
Copy Markdown
Contributor Author

The two inline findings are addressed in commit d0f90d18a:

  • A successful GitHub Copilot response whose filtered model catalog is empty is now authoritative. Storage accepts the empty catalog only for Copilot and clears stale enabled/default selections; other providers retain the non-empty contract.
  • Nullable or malformed policy values are ignored as unavailable entries instead of being dereferenced and aborting discovery.

Added Host, storage, protocol, and provider-contract regressions. Validation: Runtime provider matrix 139/139, storage focused tests 9/9, Host protocol 4/4, Host coordinator 30/30, Biome, git diff --check, and ASF header checks pass. The hosted test status previously reported failure from the original unformatted test files; the changed files are now formatted.

@seekskyworld
seekskyworld force-pushed the fix/github-copilot-model-catalog-authoritative branch from d0f90d1 to 5cca2f9 Compare September 7, 2026 04:50
@seekskyworld

Copy link
Copy Markdown
Contributor Author

Rebased the PR onto current upstream/main (b06eb02e6) and addressed the hosted protocol epoch guard. Because the Copilot empty-catalog path now emits connection.models.fetch with modelCount: 0, the Runtime Host compatibility epoch is advanced from 121 to 122 in signed commit 5cca2f95d, with the decoder compatibility reason documented.

The new hosted CI run 34084614814 is green. Local verification after a clean dependency-ordered build: Runtime Host coordinator 30/30, connection-effects protocol 4/4, Storage model facts 9/9, provider contract 1/1, Biome, git diff --check, and staged epoch guard pass.

@seekskyworld
seekskyworld force-pushed the fix/github-copilot-model-catalog-authoritative branch from 5cca2f9 to 7b1e03d Compare September 7, 2026 17:21
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
Generated-by: Codex
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
Signed-off-by: seekskyworld <djh1813553759@gmail.com>
@seekskyworld
seekskyworld force-pushed the fix/github-copilot-model-catalog-authoritative branch from 7b1e03d to 0aab913 Compare September 7, 2026 17:59
@seekskyworld

Copy link
Copy Markdown
Contributor Author

Rebased onto current upstream/main at 00f2c9e0e and pushed head 0aab913765d8c7452e816d85a6fa3e4e63e7cf2c. The protocol history now preserves upstream epoch 130 (bounded turn failure diagnostics) and records the Copilot authoritative-empty-catalog change as epoch 131. Local validation passed: dependency-ordered Core/Storage/MCP/Runtime/Runtime Host builds; connection coordinator 30/30; connection-effects protocol 4/4; Runtime Host protocol 81/81; Storage model-facts coverage 12/12; provider contract 128/128; model fetcher 13/13; Biome on all 9 changed files; and git diff --check. Hosted test is running for this new head.

@seekskyworld

Copy link
Copy Markdown
Contributor Author

The hosted test check for head 0aab913765d8c7452e816d85a6fa3e4e63e7cf2c is now green: https://github.com/apache/maka/actions/runs/34149806174/job/101829514038. No new review or inline findings were added after the rebase; GitHub reports the PR mergeable (blocked only by repository review policy).

@hqhq1025 hqhq1025 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.

Re-reviewed exact head 0aab913765d8c7452e816d85a6fa3e4e63e7cf2c. The two previously reported defects are fixed: empty filtered Copilot catalogs can now commit, and nullable/malformed policy values no longer crash discovery. One first-login correctness issue remains: a valid non-empty account catalog can be persisted with no enabled model and no default. The branch is also currently conflicting with main, where #4697 has already been closed by #4698, so it needs to be reconciled with that implementation before merge.\n\nVerification: clean dependency install and patch application; build:test; Runtime 3,310 passed / 13 skipped; Storage 1,188 passed / 10 skipped; focused Runtime Host 115/115; focused Runtime/Storage suites; typecheck, lint, format, ASF headers, and diff check. The full Runtime Host run had 1 failure in the managed Bash sandbox assertion; the same failure reproduces on the exact base 00f2c9e0, so it is not attributable to this PR. I did not call the live Copilot service; the account-catalog behavior was exercised through the real Runtime -> Host coordinator -> file-backed Storage path with a deterministic HTTP response.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

const isGitHubCopilot = previous.providerType === 'github-copilot';
const liveModelIds = isGitHubCopilot ? new Set(result.models.map(({ id }) => id)) : undefined;
const enabledModelIds = isGitHubCopilot
? reconciled.enabledModelIds.filter((modelId) => liveModelIds?.has(modelId))

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.

[P1] Seed the first authoritative catalog instead of emptying a new connection. A newly created interactive Copilot connection starts with shipped fallback IDs in enabledModelIds but models: []. When its first valid /models response contains only account models outside that fallback list, the generic reconciler preserves the fallback selection and this filter removes all of it. Through the real OAuth-authority -> Host coordinator -> file-backed Storage path, a one-model response committed with modelCount: 1 and models: [account-only-model], but persisted enabledModelIds: [] and defaultTarget: null. synchronizeRuntimeHostAccountConnectionById then has no enabledModelIds[0] to select, so login reports success while the connection cannot start a session until manually edited. Distinguish the first account-scoped fetch (previous.models.length === 0) from later refreshes: seed the live catalog on the first fetch, while preserving intersection-only behavior once an inventory already exists.

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