Repository navigation
feat: consolidate MCP OAuth sign-in - #14724
Merged
Merged
Conversation
Port the JetBrains MCP OAuth sign-in UX to the VS Code extension, adapted for the webview-based Marketplace and settings surfaces. - New McpAuthService (src/services/mcp-auth/) owns sign-in state for the whole extension: single-flight per directory+server, a 6-minute client timeout kept above the CLI's 5-minute OAuth callback timeout, cancel via credential deletion, and a status normalizer that recognizes recoverable `failed` errors (expired tokens, HTTP 401/403) as `needs_auth` so Settings, the prompt, and Marketplace agree on which servers need sign-in. - Chat prompt toolbar shows a warning icon with a per-server action menu (Sign In / Open in Settings) when any MCP server needs sign-in, deep-linking into Settings > Agent Behaviour > MCP Servers with the matching row expanded and scrolled into view. - Marketplace install flow shows an inline sign-in step after installing an OAuth-protected MCP server, with progress, cancel, and outcome handling. - Settings MCP rows gained Sign In/Cancel and Reset sign-in actions, a tooltip with the underlying failure reason, and OAuth client configuration (automatic/disabled/custom) in the MCP edit view. - Removing a companion skill that belongs to a Marketplace MCP bundle now offers to remove the whole bundle, matching the existing MCP-side removal behavior. - Fixed a pre-existing bug where editing a project-scoped MCP server's URL/command silently relocated it to the global config.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previously reported WARNING in Files Reviewed (2 files)
Previous Review Summaries (17 snapshots, latest commit bedbec8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bedbec8)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit c58468b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (14 files)
Fix these issues in Kilo Cloud Previous review (commit b51083d)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 2e15571)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit 5c75a3e)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 879d0d8)Status: No Issues Found | Recommendation: Merge The incremental commit Files Reviewed (1 file)
Previous review (commit ee065d3)Status: No Issues Found | Recommendation: Merge The incremental commit Files Reviewed (3 files)
Previous review (commit 36df73b)Status: No Issues Found | Recommendation: Merge The previous round's suggestion (double Files Reviewed (4 files)
Previous review (commit ce39379)Status: 1 Issue Found | Recommendation: Merge with minor suggestions Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Previous review (commit 9884398)Status: No Issues Found | Recommendation: Merge Incremental review at Files Reviewed (4 files)
Previous review (commit 6abbb4b)Status: No Issues Found | Recommendation: Merge Incremental review at Files Reviewed (15 files + i18n/tests)
Previous review (commit 38d30f7)Status: No Issues Found | Recommendation: Merge Incremental review at Files Reviewed (2 files)
Previous review (commit edcc43f)Status: No Issues Found | Recommendation: Merge The follow-up adds the MCP state/actions to the Storybook Files Reviewed (2 files)
Previous review (commit 7ceaadf)Status: No Issues Found | Recommendation: Merge Incremental review at Files Reviewed (18 files)
Previous review (commit 50764ff)Status: No Issues Found | Recommendation: Merge All nine findings from the previous review are resolved in Resolved:
Files Reviewed (17 files)
Previous review (commit c9632e4)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (25 files)
Fix these issues in Kilo Cloud Previous review (commit 72fa65f)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (122 files)
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by deepseek-v4.1-flash · Input: 56.5K · Output: 6.2K · Cached: 401.4K Review guidance: REVIEW.md from base branch |
Move authentication-failure classification and browser ownership into the CLI protocol so clients no longer duplicate heuristics or open browsers on the backend machine. Add a non-destructive cancel endpoint, emit authorization URLs for client-driven flows, regenerate the SDK, and migrate VS Code to the typed contract while leaving the API ready for JetBrains adoption.
kirillk
changed the title
Contributor
Author
|
Addressed all findings from the review summary and replied on each inline thread with the specific resolution. The fixes cover scope-safe MCP edits, status refresh/preservation, stale Marketplace outcomes, one-shot Settings focus, row/ref cleanup, observable eviction coverage, dead cache removal, and OAuth-disabled classification. I also updated the event-manifest and existing OAuth auto-connect expectations exposed by CI. Focused core and VS Code suites, typechecks, lint, compile, knip, annotation, and Promise-facade guards pass. |
Propagate MCP removal progress and completion across all Kilo webviews, prune removed servers from every config layer, and invalidate Agent Behaviour tabs after Marketplace or config mutations so each tab reloads authoritative data on its next activation. Also simplify the Marketplace install result and MCP auth recovery presentation.
Refresh MCP status and Agent Behaviour state after Marketplace authentication, propagate install and removal events across all Kilo webviews, require every removal scope to succeed, and suppress stale config snapshots until backend removal is confirmed.
Replace the per-issue submenu with a single-level menu that labels each issue group and separates issues with a divider, and style it with VS Code's native menu tokens. The submenu required a second click and, in the narrow sidebar, opened a panel overlapping its own trigger.
Planning artifacts under .kilo/plans/ are local scratch notes and should not ship with the branch.
marius-kilocode
approved these changes
Oct 5, 2026
# Conflicts: # packages/kilo-vscode/webview-ui/src/context/session.tsx
# Conflicts: # packages/kilo-vscode/webview-ui/src/context/config.tsx
…rning Overlay the session-issues warning icon in the top-right corner of the prompt text input instead of the bottom toolbar row, matching the JetBrains plugin's placement over the editor. Insets align with the text's top padding and the toolbar's right padding. Fix a CSS cascade bug where the icon wrapper's own color token overrode the inherited warning-yellow color, leaving the triangle rendered in the default muted tone while only its status dot picked up the warning color. Remove that now-redundant status dot, since JetBrains' icon has none. Add a new warning-filled icon for this trigger instead of changing the shared warning icon, which is used broadly elsewhere (toasts, PR badges, error display, permission dock, migration wizard) and must keep its outline style there.
packages/ui is shared upstream code; the new entries added for the session-issues warning icon were missing the required kilocode_change marker, failing the annotation check in CI.
Removing an MCP server (from the Marketplace panel or Settings) only emitted mcpRemoval bus events; it never refreshed the shared McpAuthService's needs-auth state. A server removed while still listed as needing sign-in left its session-issues warning icon stuck in the chat prompt, since the webview's stale needsAuth entry was never invalidated. Both removal paths now call the same mcpAuth broadcast already used elsewhere to revalidate sign-in state: - MarketplacePanelProvider.remove() calls mcpAuth(connection).refresh(), mirroring the exact call install() already makes after a successful install. - KiloProvider.handleRemoveMcp() calls refreshMcpAuthConsumers(), mirroring handleSignInMcp/handleResetMcpAuth. Both route through McpAuthService's shared onChange broadcast, so every provider sharing one CLI connection (sidebar, chat tabs, Settings, Marketplace) picks up the cleared state the same way an install already does.
Removing an MCP server via the marketplace (or Settings) only wrote the config file; it never told the live MCP service. status() resurrects a removed server's last-known state from two in-memory caches that disconnect() only partially clears and nothing else purges, so a server that had reached needs_auth kept reporting that status forever, even after the config entry, companion skill, and receipt were all correctly removed from disk. Add MCP.Interface.remove(name), which purges clients/defs/instructions/ status/config for a server (unlike disconnect, which only disables it). MarketplaceInstaller.removeMcp now calls it after a successful removal, via a new Services.mcp field threaded through from the kilocode HTTP handlers rather than Effect context, so install()/remove() stay usable from callers without the full MCP layer. Verified against a real installed MCP server (config entry, companion skill, and marketplace receipt) through both a direct Effect call and the live /kilocode/marketplace/remove HTTP endpoint: the server no longer reappears in status() or marketplace metadata after removal. Also syncs bun.lock's stale kilo-jetbrains version entry (7.8.1) with the already-bumped package.json (7.8.3).
mock.module replaces a module for every test file in the same bun test process, not just the file that calls it. Mocking src/kilo-provider/remove-config-item made remove-config-item.test.ts's own coverage of the real removeMcp()/removeMarketplaceItemFromAllScopes() wiring silently exercise the stub instead, breaking two of its assertions whenever both files run in one bun test invocation (as CI's unit tests job does, but not the per-file runs used earlier). Override marketplace.remove directly on the KiloProvider instance's real MarketplaceService instead, matching the pattern already used in marketplace-panel-mcp-auth-refresh.test.ts.
removeMarketplaceItemFromAllScopes calls remove() for both project and global scope unconditionally, so the scope without the entry hits removeMcp()'s no-op branch (no receipt, no matching config key). svc.mcp.remove(item.id) ran unconditionally there too, incorrectly purging the runtime status of a server actually defined in the other scope and leaving it stuck disabled until something reconnects it. Track whether this scope's receipt-removal or writeMcp() branch actually ran, and only purge runtime status when it did. Addresses review feedback from kilo-code-bot on PR #14724.
kirillk
enabled auto-merge
October 5, 2026 19:35
This was referenced Oct 6, 2026
This was referenced Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
OAuth-protected MCP servers need a sign-in flow that works consistently across the CLI and IDE clients. Previously, authentication-failure classification and browser ownership lived in each IDE: VS Code and JetBrains maintained duplicate error-string heuristics, clients cancelled by deleting stored credentials, and the CLI tried to open the browser on the machine hosting the backend. That could present the wrong recovery state, destroy credentials during cancellation, and cannot reliably open the browser on the user's frontend machine in split/remote setups.
Why This Change Was Made
This consolidates the protocol and state decisions in the CLI while keeping presentation in each client. The CLI now owns authentication-failure classification, exposes an external-browser authorization mode, emits the authorization URL over the existing event stream, and provides a non-destructive cancel endpoint. VS Code adopts that contract immediately; the API is intentionally complete for the JetBrains frontend to adopt later without another CLI change.
Changes:
needs_authwith the original error for diagnostics.POST /mcp/:name/auth/authenticateacceptsexternal: trueand emitsmcp.auth.urlinstead of opening a browser server-side.POST /mcp/:name/auth/cancel, which stops the pending flow without deleting stored credentials; credential removal remains a separate reset/sign-out operation.vscode.env.openExternal, retain the copy-URL fallback, and stop maintaining client-side authentication-error heuristics.User Impact
VS Code users get the same guided MCP sign-in workflow, but browser authorization is now opened by VS Code on the frontend machine, cancellation no longer removes credentials, and 401/403/token failures are consistently reported as requiring sign-in by the CLI itself. TUI/CLI authentication keeps its existing server-side browser behavior. The new API is ready for JetBrains to use later, including split-mode browser ownership.
Screenshots / Video
Evidence
bun test ./test/kilocode/mcp/auth-failure.test.ts ./test/kilocode/mcp/auth-classification.test.ts ./test/mcp/oauth-browser.test.ts ./test/server/httpapi-mcp-oauth.test.ts(packages/opencode/): passed, 14 tests.bun test tests/unit/mcp-auth-service.test.ts tests/unit/mcp-auth-status.test.ts tests/unit/kilo-provider-mcp-messages.test.ts tests/unit/session-issues.test.ts tests/unit/session-issues-render.test.ts tests/unit/marketplace-install-modal.test.ts tests/unit/marketplace-bundles.test.ts tests/unit/i18n-keys.test.ts tests/unit/i18n-unused-keys.test.ts(packages/kilo-vscode/): passed, 53 tests.bun run typecheck(packages/opencode/) andbun run typecheck && bun run lint(packages/kilo-vscode/): passed.bun run compile,bun run knip, andbun run check-kilocode-change(packages/kilo-vscode/): passed.bun run typecheck(repo root): passed across all 30 packages, including the regenerated SDK and JetBrains OpenAPI/typecheck tasks. The pre-push hook repeated the monorepo and JetBrains typechecks successfully before pushing.bun run lint(repo root): completed with 0 errors (existing warnings only).bun run script/check-opencode-annotations.ts --worktree,bun run script/check-opencode-promise-facades.ts,bun run script/check-md-table-padding.ts, andgit diff --check: passed.packages/opencodetimed out after 10 minutes amid unrelated PTY permission, provider-environment, and EffectManagedRuntimefailures;packages/kilo-vscodehit the existing browser-proxy fixture/Promise.withResolversenvironment failure and timed out. All focused tests for the changed paths pass as listed above.@kilocode/cli(new typed auth protocol) and minor forkilo-code(OAuth sign-in UX).Dependency
None. JetBrains adoption is follow-up work; the API required for it ships in this pull request.