fix(model-requirements): remove stale github-copilot from gpt-5-nano fallback (fixes #3359)#3377
fix(model-requirements): remove stale github-copilot from gpt-5-nano fallback (fixes #3359)#3377MoerAI wants to merge 3 commits intocode-yeongyu:devfrom
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Safe removal of an invalid provider ('github-copilot') from the fallback list for 'gpt-5-nano', preventing potential routing errors.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ebb434a6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/shared/model-requirements.ts
Outdated
| { providers: ["opencode-go"], model: "kimi-k2.5" }, | ||
| { providers: ["zai-coding-plan"], model: "glm-4.6v" }, | ||
| { providers: ["openai", "github-copilot", "opencode"], model: "gpt-5-nano" }, | ||
| { providers: ["openai", "opencode"], model: "gpt-5-nano" }, |
There was a problem hiding this comment.
Removing github-copilot from the multimodal-looker gpt-5-nano fallback entry changes a tested contract, but this commit does not update the corresponding assertions, so CI goes red immediately (bun test src/shared/model-requirements.test.ts fails at src/shared/model-requirements.test.ts:147 expecting ['openai','github-copilot','opencode']). Please update the affected tests (or adjust the invariant) in the same change so the commit remains mergeable.
Useful? React with 👍 / 👎.
|
CI test failure: the model-fallback snapshot tests reference |
MoerAI
force-pushed
the
fix/cleanup-stale-gpt5-nano-refs
branch
from
0ebb434 to
effa52b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: effa52bb5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { providers: ["opencode-go", "vercel"], model: "kimi-k2.5" }, | ||
| { providers: ["zai-coding-plan", "vercel"], model: "glm-4.6v" }, | ||
| { providers: ["openai", "github-copilot", "opencode", "vercel"], model: "gpt-5-nano" }, | ||
| { providers: ["openai", "opencode", "vercel"], model: "gpt-5-nano" }, |
There was a problem hiding this comment.
Removing github-copilot from the multimodal-looker gpt-5-nano entry leaves that agent with no matching provider when a user has only Copilot enabled, so generateModelConfig() falls through to ULTIMATE_FALLBACK (opencode/gpt-5-nano) at src/cli/model-fallback.ts:183-190, producing a config that points at an unavailable provider for that environment. I verified this regression by running bun test src/cli/model-fallback.test.ts: this commit introduces snapshot failures in the Copilot-only and mixed-provider scenarios, while the parent commit passes the same test file.
Useful? React with 👍 / 👎.
MoerAI
force-pushed
the
fix/cleanup-stale-gpt5-nano-refs
branch
from
effa52b to
7d98ab0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d98ab011c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mcpConfig = mcpJsonMcp || frontmatterMcp | ||
|
|
||
| const baseName = data.name || options.defaultName | ||
| const baseName = data.name == null ? options.defaultName : String(data.name) |
There was a problem hiding this comment.
Using data.name == null ? ... : String(data.name) treats empty-string frontmatter names as valid, so a skill with name: "" now gets registered with an empty command name instead of falling back to the file/directory-derived defaultName. This can create unnamed skills and key collisions during deduplication/lookup (skills.find(s => s.name === name)), whereas the previous || behavior safely used the inferred name for blank values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d003ac01e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mcpConfig = mcpJsonMcp || frontmatterMcp | ||
|
|
||
| const baseName = data.name || options.defaultName | ||
| const baseName = data.name == null ? options.defaultName : String(data.name) |
There was a problem hiding this comment.
Converting every non-null data.name with String(data.name) turns invalid YAML values like name: false into the literal command name "false" instead of using defaultName. Before this change, falsy non-string values fell back to the inferred directory/file name, which avoids accidental collisions and unusable command names when frontmatter is malformed.
Useful? React with 👍 / 👎.
1 participant
Summary
github-copilotprovider from thegpt-5-nanofallback entry in multimodal-looker agentProblem
The
multimodal-lookeragent's fallback chain includesgithub-copilotas a provider forgpt-5-nano, butgpt-5-nanois not available in the GitHub Copilot model catalog (opencode models github-copilot --verbosedoes not list it). This creates confusion during debugging and model selection.Fix
Removed
github-copilotfrom the providers array for thegpt-5-nanofallback entry inAGENT_MODEL_REQUIREMENTS.multimodal-looker. Theopenaiandopencodeproviders remain as valid sources for this model.Changes
src/shared/model-requirements.tsgithub-copilotfrom multimodal-looker gpt-5-nano fallback providersFixes #3359
Summary by cubic
Moved
github-copilotfrom thegpt-5-nanofallback togpt-5.4for themultimodal-lookeragent to match the model catalog and reduce confusion. Also coerces numeric skill frontmatter names to strings and regenerates snapshots (fixes #3359).AGENT_MODEL_REQUIREMENTS.multimodal-looker:gpt-5.4providers are now["openai", "github-copilot", "opencode", "vercel"];gpt-5-nanoproviders are["openai", "opencode", "vercel"].frontmatter.nameto a string to support numeric names.github-copilot/gpt-5.4(variantmedium); added test verifying numeric names load as strings.Written for commit 8d003ac. Summary will update on new commits.