feat: support adding GitHub Copilot AI provider via UI#25888
Conversation
ebb97fa to
193fd44
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.6.0 | Round 1 | Last posted: Round 1, 13 findings (2 P2, 6 P3, 4 Nit, 1 Note), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 2 P2, 6 P3, 4 Nit, 1 Note new. 1 dropped. Reviewed against 5af0214..193fd44. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean implementation that mirrors the Bedrock sentinel pattern well. Defense-in-depth for key rejection is solid: SDK validation on create, transaction sentinel on update, frontend schema omitting the field, frontend mapping stripping keys from the wire payload. Four independent layers. The copilot wire type is preserved end-to-end without collapsing to openai, which is the correct architectural call.
2 P2, 6 P3, 4 Nit, 1 Note.
The two P2s are a functional bug (the submit button is disabled on a valid copilot create form because the dirty gate fires with all defaults pre-filled) and an endpoint default mismatch between the UI and backend. Both are straightforward fixes.
Most P3s cluster around missing Storybook coverage for the copilot edit flow, which 9 reviewers independently flagged. The icon, row, and page-level edit stories all need a copilot variant to match the convention.
"Boring code gets silence. The rest holds up." (Hisoka)
site/src/pages/AISettingsPage/ProvidersPage/components/ProviderForm.tsx:442
P2 [CRF-1] The submit button is permanently disabled on a valid copilot create form.
The button is gated on !form.dirty, and Formik computes dirty by comparing current values against initial values. For every other provider type, at least one mandatory field starts empty (API key for openai/anthropic, access key pair for bedrock), so filling it naturally makes the form dirty. Copilot is the first provider whose providerDefaults (name: "copilot", baseUrl: "https://api.business.githubcopilot.com") satisfy the entire makeCopilotSchema on mount.
Trace: AddProviderPage passes initialValues={{ type: "copilot" }}. ProviderForm merges defaultInitialValues < typeDefaults < initialValues, producing { name: "copilot", baseUrl: "https://api.business.githubcopilot.com", ... }. form.isValid is true, form.dirty is false, button stays disabled. The user sees a complete form they cannot submit without making a throwaway edit.
Fix: either start with name: "" in copilot's providerDefaults (so the user must type a name, making the form dirty), or gate the button on !form.dirty only when editing is true. (Nami)
🤖
site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.ts:92
P3 [CRF-5] getProviderDisplayType falls through to host matching for copilot, where every other non-collapsing type has an early return.
Bedrock (line 95) and Anthropic (line 98) both return before the host-matching logic. Copilot passes through and relies on the provider.type fallback at line 103. With the default api.business.githubcopilot.com this works (no host match). But the PR says users on other tiers will edit the endpoint. If an admin pointed it at a URL whose host coincidentally matched an Azure subdomain, the list view renders the Azure icon instead of Copilot.
Fix: add if (provider.type === "copilot") return "copilot"; after the anthropic guard, consistent with the bedrock/anthropic pattern. (Hisoka)
🤖
site/src/pages/AISettingsPage/ProvidersPage/components/ProviderForm.tsx:345
P3 [CRF-7] The endpoint field gives no copilot-specific guidance on valid values or tier selection.
Copilot has three distinct tiers with three different hostnames (api.individual.githubcopilot.com, api.business.githubcopilot.com, api.enterprise.githubcopilot.com). Compare with Bedrock, which gets a custom description ("In the format of https://bedrock-runtime.{region}.amazonaws.com"). The admin has to know which URL to use from external context; getting it wrong produces a runtime failure, not a validation error.
Consider overriding the endpoint description when the selected type is copilot to list the three valid hostnames, similar to the Bedrock format hint. (Pen Botter)
🤖
site/src/pages/AISettingsPage/ProvidersPage/UpdateProviderPage/UpdateProviderPageView.tsx:46
Note [CRF-13] providerIsOpenAiAnthropic is now inaccurate for copilot.
The variable is !isBedrockProvider(provider), which evaluates to true for copilot. Before this PR, "not Bedrock" was synonymous with "uses API keys." Copilot is the first type to break that equivalence. The behavior is safe today: provider.api_keys.length > 0 independently gates credential seeding (line 112), so openAiAnthropicSavedApiKey resolves to false and no masked credential is seeded. But the variable's contract now lies to the reader.
9 of 22 reviewers independently flagged this. The code is correct; only the naming misleads. A rename to providerUsesApiKeys (with && provider.type !== "copilot") would make the intent explicit, but this file is outside the diff. (Chopper, Hisoka, Kite, Knov, Luffy, Meruem, Razor, Pen Botter, Zoro)
🤖
🤖 This review was automatically generated with Coder Agents.
Resolve the deep-review findings on PR #25888: - Gate the submit button's !form.dirty check to edit mode so a valid create form (Copilot prefills every required field) is submittable without a throwaway edit (CRF-1). - Align the UI default Copilot endpoint with the backend and docs (individual tier) (CRF-2). - State the GitHub external-auth prerequisite in the Copilot help text (CRF-3). - Add edit-mode Storybook coverage for Copilot (ProviderForm, UpdateProviderPageView) and a ProviderIcon Copilot story (CRF-4, CRF-11). - Return early for Copilot in getProviderDisplayType (CRF-5). - Trim duplicated Copilot auth comments, keeping the canonical sentinel doc and type const doc (CRF-6). - Add Copilot endpoint tier guidance listing the three hostnames (CRF-7). - Extract the shared base object in providerFormValuesToCreate (CRF-8). - Mention Copilot in the CreateAIProviderRequest doc comment (CRF-9). - Update the "unreachable" comment to mention Copilot (CRF-10). - Pass only the type in the AddCopilot story to match the real add flow (CRF-12). - Rename providerIsOpenAiAnthropic to providerUsesApiKeys and exclude Copilot so the contract is honest (CRF-13).
5af0214 to
70e8be7
Compare
6e916b4 to
23b60ef
Compare
98e8207 to
23b60ef
Compare
ibetitsmike
left a comment
There was a problem hiding this comment.
approved with a nit
| // maxLoggedErrorBodyBytes bounds how much of an aibridged error response | ||
| // body is rendered into a log line, so a large upstream error payload | ||
| // cannot blow up log volume. | ||
| const maxLoggedErrorBodyBytes = 16 << 10 // 16 KiB |
There was a problem hiding this comment.
fancy, but isn't 16*1024 just more readable? or even explicitly adding 16384
23b60ef to
8bea045
Compare
70e8be7 to
1e0ff3a
Compare
8bea045 to
2e2fe02
Compare
1e0ff3a to
72fc38b
Compare
Merge activity
|
Copilot was supported by the aibridge runtime and env-var seeding but blocked from the runtime CRUD API and absent from the /ai/settings UI, because its auth model (per-request GitHub OAuth tokens, no stored key) did not fit the credential-centric add-provider flow. Allow type=copilot in CreateAIProviderRequest.Validate() and reject api_keys for copilot on both create and update, mirroring the bedrock guards. Add copilot to the provider type picker and give the form a credential-free branch defaulting to the business endpoint, mapping it to the distinct copilot wire type rather than collapsing to openai. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When aibridged returns a 4xx or 5xx response, the proxy now logs the response body alongside the status. The body is read and then restored with an equivalent reader so the proxy still forwards it downstream and the response dumper can read it again. The logged rendering is capped at 16 KiB so a large upstream error payload cannot blow up log volume. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the deep-review findings on PR #25888: - Gate the submit button's !form.dirty check to edit mode so a valid create form (Copilot prefills every required field) is submittable without a throwaway edit (CRF-1). - Align the UI default Copilot endpoint with the backend and docs (individual tier) (CRF-2). - State the GitHub external-auth prerequisite in the Copilot help text (CRF-3). - Add edit-mode Storybook coverage for Copilot (ProviderForm, UpdateProviderPageView) and a ProviderIcon Copilot story (CRF-4, CRF-11). - Return early for Copilot in getProviderDisplayType (CRF-5). - Trim duplicated Copilot auth comments, keeping the canonical sentinel doc and type const doc (CRF-6). - Add Copilot endpoint tier guidance listing the three hostnames (CRF-7). - Extract the shared base object in providerFormValuesToCreate (CRF-8). - Mention Copilot in the CreateAIProviderRequest doc comment (CRF-9). - Update the "unreachable" comment to mention Copilot (CRF-10). - Pass only the type in the AddCopilot story to match the real add flow (CRF-12). - Rename providerIsOpenAiAnthropic to providerUsesApiKeys and exclude Copilot so the contract is honest (CRF-13).
Signed-off-by: Danny Kopping <danny@coder.com>
2e2fe02 to
7a08f86
Compare
Copilot is the only AI provider type that could not be added through the
/ai/settingsUI. The aibridge runtime and the env-var seeding path already supported it, but the runtime CRUD API rejectedtype=copilotand the UI omitted it entirely. The root cause is that Copilot's auth model (a per-request GitHub OAuth token, with no pre-shared key) does not fit the credential-centric add-provider flow that every other provider uses.Backend
Allow
type=copilotinCreateAIProviderRequest.Validate(), and rejectapi_keysfor Copilot on both create (validation) and update (handler sentinel), mirroring the existing Bedrock guards. Copilot carries no stored credential.Frontend
Add Copilot to the provider type picker (with the
github-copilot.svgicon) and give the form a credential-free branch: name, display name, and a free-text endpoint defaulting tohttps://api.business.githubcopilot.com, with copy explaining that authentication happens via the user's GitHub token at request time. Copilot maps to the distinctcopilotwire type rather than collapsing toopenai, and the edit flow recovers it correctly.The endpoint stays required with a business-tier default; users on the individual or enterprise endpoints edit the field.
🤖 Generated with Claude Code