From 8da537af83b35990b0c05e17de9f7c156acffb46 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 11:05:34 +0000 Subject: [PATCH 1/6] Detect the ChatGPT desktop app as a partially-supported client Reimplements Edison-Watch/edison-watch#1064 against the current architecture. That PR landed in the old `client_2/` tree, where the Electron main process probed for clients itself; detection has since moved into the detector daemon, so the same behaviour is rebuilt here rather than ported line for line. ChatGPT exposes MCP through server-side Connectors, hosted in the OpenAI account rather than in a local config file. Edison can see the app is installed but cannot read, write, hook, or proxy anything for it, so it belongs in the wizard's existing "we only support local MCP servers, not Connectors" section next to Claude Desktop and Claude Cowork. Detection (daemon): - New `ChatGpt` agent, behind a `chatgpt` cargo feature. Presence is probed from the app itself, not a config path: `ChatGPT.app` / `ChatGPT Classic.app` on macOS (post-merger the unified Chat + Work + Codex app ships as `ChatGPT.app`), the Store execution alias and a direct install on Windows, nothing on Linux. It discovers no servers and is never an install target. Advisory-only in the app: - `chatgpt` added to `McpClientId` + `CLIENT_DISPLAY`, marked `connectorOnly` with a label to show where a config path would go. - `applyIntegrations` drops connector-only clients, so the wizard's default "select everything" never asks the daemon to install into an app with no config file. - Setup status is reported over the managed clients only. Both answers it could give for ChatGPT mislead: "gateway not configured" blames the user for something they can't fix, and "nothing applicable" paints an unprotected app green. - `readConfig` explains the Connectors situation instead of surfacing the daemon's "no user-scope config" error. - Wizard: ChatGPT joins the partially-supported set; fixed the banner's "a equivalent" typo. The Codex CLI stays a separate, fully-supported client. Testing: desktop typecheck + 18 vitest files pass; new `connectorOnlyClients.test.ts` pins the no-install / no-status behaviour; new Rust unit tests cover the probe. Detection itself is verified by tests and logic-trace, not a live run - CI here can't launch macOS/Windows or the real Electron binary. Note that `cargo test` for the full daemon can't run in this sandbox: `libsqlite3-sys` 0.38.1's build script needs a newer stable toolchain than the one installed, a pre-existing condition unrelated to this change. The `edison-detectord` lib, its tests, clippy and rustfmt all pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LGqPs27HH3TkQ6hQeJ5mAK --- .../crates/edison-detectord/Cargo.toml | 4 + .../edison-detectord/src/clients/chatgpt.rs | 152 ++++++++++++++++++ .../edison-detectord/src/clients/mod.rs | 7 + .../crates/mcp_detector_daemon/src/agents.rs | 7 +- packages/desktop/README.md | 2 + .../__tests__/connectorOnlyClients.test.ts | 67 ++++++++ .../desktop/src/main/clients/displayMeta.ts | 20 +++ packages/desktop/src/main/clients/registry.ts | 19 +++ .../src/main/detectord/integrations.ts | 16 +- packages/desktop/src/main/discovery/types.ts | 4 + packages/desktop/src/main/ipc/ipcHandlers.ts | 4 +- .../src/main/ipc/ipcHandlersMcpSubmit.ts | 12 ++ .../desktop/src/main/runtime/hookStatus.ts | 6 +- .../onboarding/AppsStep.stories.tsx | 13 ++ .../src/components/onboarding/AppsStep.tsx | 8 +- 15 files changed, 333 insertions(+), 8 deletions(-) create mode 100644 crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs create mode 100644 packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts diff --git a/crates/detectord/crates/edison-detectord/Cargo.toml b/crates/detectord/crates/edison-detectord/Cargo.toml index c20c555..d91b4d0 100644 --- a/crates/detectord/crates/edison-detectord/Cargo.toml +++ b/crates/detectord/crates/edison-detectord/Cargo.toml @@ -22,6 +22,7 @@ default = [ "zed", "jetbrains", "codex", + "chatgpt", ] # Each agent can be opted into independently. `vscode` and `cursor` pull in # rusqlite (for reading `state.vscdb`) + serde_json_lenient (JSONC); `codex` @@ -35,6 +36,9 @@ claude_cowork = [] windsurf = [] zed = [] jetbrains = [] +# Presence detection only: ChatGPT keeps its MCP servers as server-side +# Connectors, so there is no config to parse and no parser dep to pull in. +chatgpt = [] [dependencies] dirs = "6.0.0" diff --git a/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs b/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs new file mode 100644 index 0000000..b96771f --- /dev/null +++ b/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs @@ -0,0 +1,152 @@ +//! ChatGPT desktop app [`Agent`] — presence detection only. +//! +//! ChatGPT's MCP servers are **Connectors**: they are configured in the OpenAI +//! account and run server-side, so unlike every other agent here there is no +//! local config file. Nothing to watch, nothing to discover, and nowhere to +//! install the `edison-watch` entry. +//! +//! It is still worth reporting, because the app uses "is it installed?" to warn +//! the user that their ChatGPT connectors are outside Edison's reach — the same +//! bucket as the Claude hosts' connectors, minus even the local file those have +//! to fall back on. Detection therefore keys off the *app bundle / executable* +//! rather than a config path. + +use std::path::PathBuf; + +use crate::agent::Agent; +use crate::error::Result; +use crate::types::DiscoveredServer; +use crate::watch::WatchTargets; + +const CLIENT_NAME: &str = "chatgpt"; + +pub struct ChatGpt { + /// Places the app can live; present when any one of them exists. + candidates: Vec, +} + +impl ChatGpt { + pub fn discover() -> Result { + Ok(Self { + candidates: default_app_paths(), + }) + } + + /// Construct from explicit candidate paths (tests / non-standard installs). + pub fn from_paths(candidates: Vec) -> Self { + Self { candidates } + } +} + +impl Agent for ChatGpt { + fn name(&self) -> &'static str { + CLIENT_NAME + } + + fn is_installed(&self) -> bool { + self.candidates.iter().any(|p| p.exists()) + } + + fn watch_targets(&self) -> WatchTargets { + // No local config exists, so there is no file whose change could mean + // "a connector was added". Watching the app bundle would only report + // updates to ChatGPT itself. + WatchTargets { + files: Vec::new(), + dirs: Vec::new(), + needs_periodic_rescan: false, + } + } + + fn discover(&self) -> Result> { + // Connectors live in the OpenAI account; the daemon cannot enumerate + // them and must not imply "ChatGPT has no MCP servers" — the app says + // so explicitly in the wizard's partially-supported section instead. + Ok(Vec::new()) + } + + // `edison_installs` / `hook_install` stay at their empty defaults: there is + // no local surface to install into, so ChatGPT is never an install target. +} + +fn default_app_paths() -> Vec { + if cfg!(target_os = "macos") { + // Post-2026 Codex↔ChatGPT merger: the unified Chat + Work + Codex app + // ships as `ChatGPT.app`; the older standalone chat app is + // `ChatGPT Classic.app`. (The Codex *CLI* is a separate, fully + // supported agent — see `clients/codex.rs`.) + const NAMES: [&str; 2] = ["ChatGPT.app", "ChatGPT Classic.app"]; + let mut out: Vec = NAMES + .iter() + .map(|n| PathBuf::from("/Applications").join(n)) + .collect(); + if let Some(home) = dirs::home_dir() { + out.extend(NAMES.iter().map(|n| home.join("Applications").join(n))); + } + out + } else if cfg!(target_os = "windows") { + // Shipped through the Microsoft Store (product id 9NT1R1C2HH7J), which + // registers a `ChatGPT.exe` app-execution alias under + // `%LOCALAPPDATA%\Microsoft\WindowsApps`. The `Programs` path covers a + // direct (non-Store) install. + match dirs::data_local_dir() { + Some(local) => vec![ + local + .join("Microsoft") + .join("WindowsApps") + .join("ChatGPT.exe"), + local.join("Programs").join("ChatGPT").join("ChatGPT.exe"), + ], + None => Vec::new(), + } + } else { + // No official Linux desktop app — never detected. + Vec::new() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[test] + fn installed_when_any_candidate_exists() { + let dir = tempdir().unwrap(); + let app = dir.path().join("ChatGPT.app"); + let missing = dir.path().join("ChatGPT Classic.app"); + + assert!(!ChatGpt::from_paths(vec![app.clone(), missing.clone()]).is_installed()); + std::fs::create_dir(&app).unwrap(); + assert!(ChatGpt::from_paths(vec![app, missing]).is_installed()); + } + + #[test] + fn is_usable_as_a_shared_trait_object() { + // How the daemon holds it: `Vec>`, shared across the + // watcher's threads. Compile-time check that nothing here broke `Send`. + let agent: std::sync::Arc = + std::sync::Arc::new(ChatGpt::discover().expect("infallible")); + assert_eq!(agent.name(), CLIENT_NAME); + } + + #[test] + fn never_installed_without_candidates() { + // The Linux case: no official desktop app, so no paths to probe. + assert!(!ChatGpt::from_paths(Vec::new()).is_installed()); + } + + #[test] + fn discovers_nothing_and_is_not_an_install_target() { + let dir = tempdir().unwrap(); + let app = dir.path().join("ChatGPT.app"); + std::fs::create_dir(&app).unwrap(); + let agent = ChatGpt::from_paths(vec![app]); + + assert!(agent.is_installed()); + assert!(agent.discover().unwrap().is_empty()); + assert!(agent.edison_installs(dir.path()).is_empty()); + assert!(agent.hook_install(dir.path()).is_none()); + assert!(agent.watch_targets().files.is_empty()); + } +} diff --git a/crates/detectord/crates/edison-detectord/src/clients/mod.rs b/crates/detectord/crates/edison-detectord/src/clients/mod.rs index afb2fa0..160e10b 100644 --- a/crates/detectord/crates/edison-detectord/src/clients/mod.rs +++ b/crates/detectord/crates/edison-detectord/src/clients/mod.rs @@ -31,6 +31,11 @@ mod transport; #[cfg(any(feature = "vscode", feature = "cursor"))] mod statedb; +// ChatGPT is presence-detection only - server-side Connectors, no local config +// to parse - so it is deliberately absent from the `common`/`transport` gates +// above. +#[cfg(feature = "chatgpt")] +pub mod chatgpt; #[cfg(feature = "claude_code")] pub mod claude_code; #[cfg(feature = "claude_cowork")] @@ -50,6 +55,8 @@ pub mod windsurf; #[cfg(feature = "zed")] pub mod zed; +#[cfg(feature = "chatgpt")] +pub use chatgpt::ChatGpt; #[cfg(feature = "claude_code")] pub use claude_code::ClaudeCode; #[cfg(feature = "claude_cowork")] diff --git a/crates/detectord/crates/mcp_detector_daemon/src/agents.rs b/crates/detectord/crates/mcp_detector_daemon/src/agents.rs index 407a9fb..6d777df 100644 --- a/crates/detectord/crates/mcp_detector_daemon/src/agents.rs +++ b/crates/detectord/crates/mcp_detector_daemon/src/agents.rs @@ -4,7 +4,8 @@ use std::sync::Arc; use edison_detectord::Agent; use edison_detectord::clients::{ - ClaudeCode, ClaudeCowork, ClaudeDesktop, Codex, Cursor, JetBrains, VsCode, Windsurf, Zed, + ChatGpt, ClaudeCode, ClaudeCowork, ClaudeDesktop, Codex, Cursor, JetBrains, VsCode, Windsurf, + Zed, }; /// Discover the locally-available agents. An agent whose `discover()` @@ -29,6 +30,10 @@ pub fn build() -> Vec> { add!(Windsurf::discover(), "windsurf"); add!(Zed::discover(), "zed"); add!(Codex::discover(), "codex"); + // Detect-only: reports whether the ChatGPT desktop app is installed so the + // app can warn that its Connectors are outside Edison's reach. It + // contributes no servers and is never an install target. + add!(ChatGpt::discover(), "chatgpt"); add!(JetBrains::intellij(), "intellij"); add!(JetBrains::pycharm(), "pycharm"); add!(JetBrains::webstorm(), "webstorm"); diff --git a/packages/desktop/README.md b/packages/desktop/README.md index 25dab63..794ea73 100644 --- a/packages/desktop/README.md +++ b/packages/desktop/README.md @@ -53,6 +53,8 @@ Modern AI tools (Claude, Cursor, VS Code, and friends) connect to MCP servers th Claude Code · Claude Desktop · Claude Cowork · Cursor · VS Code · Windsurf · Zed · JetBrains IDEs · Codex +The ChatGPT desktop app is detected but not managed: its MCP servers are Connectors hosted in your OpenAI account rather than in a local config file, so the setup wizard flags them for you to remove instead of claiming to protect them. + ## Getting Started 1. **Install** the app - see [Installation](#installation) (or [build from source](#build-from-source) until signed installers ship). diff --git a/packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts b/packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts new file mode 100644 index 0000000..286a87d --- /dev/null +++ b/packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +/** + * ChatGPT is a detect-only client: its MCP servers are Connectors hosted in the + * user's OpenAI account, so there is no local config for Edison to read, write, + * hook, or proxy. The whole "surface ChatGPT in the wizard without pretending we + * protect it" design rests on that staying true, so it's pinned here - if + * ChatGPT is ever given a real config surface, these are the tests to revisit. + */ + +const applyIntegrationsRpc = vi.fn(() => Promise.resolve([])) +const connect = vi.fn(() => Promise.resolve()) + +vi.mock('../detectord/lifecycle', () => ({ + getDetectordClient: () => ({ connect, applyIntegrations: applyIntegrationsRpc }) +})) + +// health drags in electron (dialogs for the missing-binary case). +vi.mock('../detectord/health', () => ({ + withDetectordHealth: (_label: string, fn: () => Promise) => fn() +})) + +vi.mock('electron', () => ({ app: { getPath: () => '/tmp' }, BrowserWindow: { getAllWindows: () => [] } })) + +import { applyIntegrations } from '../detectord/integrations' +import { CLIENT_DISPLAY } from '../clients/displayMeta' +import { CLIENT_LIST, MANAGED_CLIENT_LIST, isConnectorOnly } from '../clients/registry' + +describe('connector-only clients', () => { + beforeEach(() => { + applyIntegrationsRpc.mockClear() + connect.mockClear() + }) + + it('describes ChatGPT as connector-only, with a label instead of a path', () => { + expect(isConnectorOnly('chatgpt')).toBe(true) + // The wizard renders this where a config path would go; blank would read as + // "we couldn't find it" rather than "there is nothing to find". + expect(CLIENT_DISPLAY.chatgpt.configLabel).toBeTruthy() + }) + + it('treats every other client as manageable', () => { + expect(isConnectorOnly('claude-code')).toBe(false) + // Claude Desktop/Cowork are "partially supported" in the wizard but DO have + // a local config Edison writes, so they stay manageable. + expect(isConnectorOnly('claude-desktop')).toBe(false) + expect(isConnectorOnly('claude-cowork')).toBe(false) + }) + + it('leaves ChatGPT out of the managed list, so it gets no setup status', () => { + expect(CLIENT_LIST.map((c) => c.id)).toContain('chatgpt') + expect(MANAGED_CLIENT_LIST.map((c) => c.id)).not.toContain('chatgpt') + }) + + it('never asks the daemon to install into ChatGPT', async () => { + // The wizard selects every detected app by default, so this is the ordinary + // case, not an edge one. + expect(await applyIntegrations(['chatgpt'])).toEqual([]) + expect(connect).not.toHaveBeenCalled() + expect(applyIntegrationsRpc).not.toHaveBeenCalled() + }) + + it('still installs the other apps selected alongside it', async () => { + await applyIntegrations(['claude-code', 'chatgpt', 'cursor']) + expect(applyIntegrationsRpc).toHaveBeenCalledWith(['claude_code', 'cursor']) + }) +}) diff --git a/packages/desktop/src/main/clients/displayMeta.ts b/packages/desktop/src/main/clients/displayMeta.ts index e79faf6..ee2dfbb 100644 --- a/packages/desktop/src/main/clients/displayMeta.ts +++ b/packages/desktop/src/main/clients/displayMeta.ts @@ -11,6 +11,20 @@ import type { McpClientId } from '../discovery/types' export interface ClientDisplay { name: string brandColor: string + /** + * The client keeps its MCP servers as hosted Connectors in the user's account + * rather than in a local config file. Edison can see the app is installed but + * has nothing to read, write, hook, or proxy - so these clients are detected + * and flagged, never managed. + */ + connectorOnly?: boolean + /** + * What to show where a config path would go, for `connectorOnly` clients. + * The daemon reports no path for them (there isn't one), and a blank line + * under the app name reads as "we couldn't find it" rather than "there is + * nothing to find". + */ + configLabel?: string } export const CLIENT_DISPLAY: Record = { @@ -25,4 +39,10 @@ export const CLIENT_DISPLAY: Record = { intellij: { name: 'IntelliJ IDEA', brandColor: '#000000' }, pycharm: { name: 'PyCharm', brandColor: '#21D789' }, webstorm: { name: 'WebStorm', brandColor: '#07C3F2' }, + chatgpt: { + name: 'ChatGPT', + brandColor: '#000000', + connectorOnly: true, + configLabel: 'Connectors · managed server-side in your ChatGPT account', + }, } diff --git a/packages/desktop/src/main/clients/registry.ts b/packages/desktop/src/main/clients/registry.ts index 6f4e47b..c6cdc3c 100644 --- a/packages/desktop/src/main/clients/registry.ts +++ b/packages/desktop/src/main/clients/registry.ts @@ -20,3 +20,22 @@ export const CLIENT_LIST: ClientEntry[] = ( ).map((id) => ({ id, display: CLIENT_DISPLAY[id] })) export const CLIENT_IDS: McpClientId[] = CLIENT_LIST.map((c) => c.id) + +/** + * The clients Edison can actually manage - i.e. everything except the + * connector-only ones (ChatGPT), whose MCP servers live in the user's account. + * + * Setup status is reported over this list rather than `CLIENT_LIST`, because + * every answer it could give for a connector-only client is wrong: "gateway not + * configured" blames the user for something they can't fix, and "nothing + * applicable, all good" paints an unprotected app green. It gets detected and + * flagged in the onboarding wizard instead. + */ +export const MANAGED_CLIENT_LIST: ClientEntry[] = CLIENT_LIST.filter( + (c) => !c.display.connectorOnly +) + +/** Whether this client is detect-only (server-side Connectors, no local config). */ +export function isConnectorOnly(clientId: string): boolean { + return CLIENT_DISPLAY[clientId as McpClientId]?.connectorOnly === true +} diff --git a/packages/desktop/src/main/detectord/integrations.ts b/packages/desktop/src/main/detectord/integrations.ts index c3d96e6..0409b3b 100644 --- a/packages/desktop/src/main/detectord/integrations.ts +++ b/packages/desktop/src/main/detectord/integrations.ts @@ -9,16 +9,28 @@ import { getDetectordClient } from './lifecycle' import { toAgentName } from './agents' import { withDetectordHealth } from './health' +import { isConnectorOnly } from '../clients/registry' import type { IntegrationChange } from './protocol' export type { IntegrationChange } -/** Install the edison-watch entry + hooks for these client ids. */ +/** + * Install the edison-watch entry + hooks for these client ids. + * + * Connector-only clients (ChatGPT) are dropped first. The wizard selects every + * detected app by default, so they arrive here routinely - and asking the + * daemon to install into one would add it to the enrolled selection, which the + * self-heal then revisits forever, all to write a config file that does not + * exist. Silently skipping matches what the user is told about them: detected, + * not managed. + */ export async function applyIntegrations(clients: string[]): Promise { + const installable = clients.filter((c) => !isConnectorOnly(c)) + if (installable.length === 0) return [] return withDetectordHealth('apply_integrations', async () => { const daemon = getDetectordClient() await daemon.connect() - return daemon.applyIntegrations(clients.map(toAgentName)) + return daemon.applyIntegrations(installable.map(toAgentName)) }) } diff --git a/packages/desktop/src/main/discovery/types.ts b/packages/desktop/src/main/discovery/types.ts index 5dbf992..7d8d0e0 100644 --- a/packages/desktop/src/main/discovery/types.ts +++ b/packages/desktop/src/main/discovery/types.ts @@ -14,6 +14,10 @@ export type McpClientId = | 'intellij' | 'pycharm' | 'webstorm' + // Detect-only: its MCP servers are server-side Connectors, so it never + // appears as the `client` of a discovered server - only in the installed-app + // list, where the wizard flags it as partially supported. + | 'chatgpt' export type McpServerTransport = 'stdio' | 'http' | 'sse' diff --git a/packages/desktop/src/main/ipc/ipcHandlers.ts b/packages/desktop/src/main/ipc/ipcHandlers.ts index cb01051..2fff743 100644 --- a/packages/desktop/src/main/ipc/ipcHandlers.ts +++ b/packages/desktop/src/main/ipc/ipcHandlers.ts @@ -415,7 +415,9 @@ export function registerIpcHandlers(deps: IpcHandlerDeps): void { clients.push({ id, name: CLIENT_DISPLAY[id]?.name ?? id, - configPath: f.configPath ?? '' + // Connector-only clients have no path to report - they get an advisory + // label saying where their MCP config actually lives instead. + configPath: f.configPath ?? CLIENT_DISPLAY[id]?.configLabel ?? '' }) } return { clients, daemonUnavailable: false } diff --git a/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts b/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts index e2f0e5f..236380c 100644 --- a/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts +++ b/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts @@ -16,6 +16,7 @@ import { import { getDetectordClient } from "../detectord/lifecycle"; import { toAgentName } from "../detectord/agents"; import { applyIntegrations, revertIntegrations, integrationErrors } from "../detectord/integrations"; +import { CLIENT_DISPLAY } from "../clients/displayMeta"; import { detectSecrets } from "../discovery/secretDetection"; import type { TemplatizedConfig } from "../discovery/secretDetection"; import { filterOutEdisonWatchServers } from "../runtime/mcpConfigMonitor"; @@ -136,6 +137,17 @@ export function registerMcpSubmitHandlers(): void { // Show an agent's config file. The daemon reads it: it owns agent files, and // this used to take an arbitrary path from the renderer. ipcMain.handle("mcp:readConfig", async (_event, client: string) => { + // A connector-only client has no local config, so asking the daemon for one + // only produces an error the user can do nothing about. Say what's actually + // going on instead. + const display = CLIENT_DISPLAY[client as McpClientId]; + if (display?.connectorOnly) { + return { + content: + `${display.name} keeps its MCP servers as Connectors in your account, not in a ` + + `local config file. There is nothing here for Edison Watch to read or protect.`, + }; + } try { const daemon = getDetectordClient(); await daemon.connect(); diff --git a/packages/desktop/src/main/runtime/hookStatus.ts b/packages/desktop/src/main/runtime/hookStatus.ts index c6ae46c..1552e61 100644 --- a/packages/desktop/src/main/runtime/hookStatus.ts +++ b/packages/desktop/src/main/runtime/hookStatus.ts @@ -10,7 +10,7 @@ import { getAgentFacts, type AgentFacts } from '../detectord/agents' import { DetectordUnavailableError } from '../discovery/mcpDiscovery' -import { CLIENT_LIST } from '../clients/registry' +import { MANAGED_CLIENT_LIST } from '../clients/registry' import type { McpClientId } from '../discovery/types' import type { ClaudeCodeMcpStatus } from '../infra/setupConfig' @@ -61,7 +61,9 @@ export async function getHookStatus( // would read as a broken installation. Say we don't know instead. if (!facts) throw new DetectordUnavailableError() - return CLIENT_LIST.map((client) => { + // Connector-only clients are excluded: there is no hook surface and no + // gateway entry to install, so any status line for them misleads. + return MANAGED_CLIENT_LIST.map((client) => { const f = facts.get(client.id) if (!f) { // The daemon didn't report this agent (unreachable, or too old to know diff --git a/packages/desktop/src/renderer/src/components/onboarding/AppsStep.stories.tsx b/packages/desktop/src/renderer/src/components/onboarding/AppsStep.stories.tsx index de4449a..56103fe 100644 --- a/packages/desktop/src/renderer/src/components/onboarding/AppsStep.stories.tsx +++ b/packages/desktop/src/renderer/src/components/onboarding/AppsStep.stories.tsx @@ -31,6 +31,19 @@ const MOCK_CLIENTS = [ name: 'Windsurf', configPath: '/Users/alice/.windsurf/mcp.json', }, + // Connector-only clients: detected, but their MCP servers live in the user's + // account, so they render in the "partially supported" section with the + // remove-your-connectors warning instead of as configurable apps. + { + id: 'claude-desktop', + name: 'Claude Desktop', + configPath: '/Users/alice/Library/Application Support/Claude/claude_desktop_config.json', + }, + { + id: 'chatgpt', + name: 'ChatGPT', + configPath: 'Connectors · managed server-side in your ChatGPT account', + }, ]; /** Two detected MCP clients ready to configure. */ diff --git a/packages/desktop/src/renderer/src/components/onboarding/AppsStep.tsx b/packages/desktop/src/renderer/src/components/onboarding/AppsStep.tsx index 2938d0f..dcecf5a 100644 --- a/packages/desktop/src/renderer/src/components/onboarding/AppsStep.tsx +++ b/packages/desktop/src/renderer/src/components/onboarding/AppsStep.tsx @@ -273,7 +273,11 @@ export default function AppsStep({ const selectedCount = clients.filter((c) => c.enabled).length; - const PARTIALLY_SUPPORTED_IDS = new Set(["claude-desktop", "claude-cowork"]); + // Clients whose MCP config lives server-side as hosted Connectors (Claude + // account / OpenAI account), not in a local file Edison can proxy. We can + // detect the app but not protect its connectors, so we surface the warning + // below and ask the user to remove connectors + request equivalents. + const PARTIALLY_SUPPORTED_IDS = new Set(["claude-desktop", "claude-cowork", "chatgpt"]); const fullySupportedClients = clients.filter((c) => !PARTIALLY_SUPPORTED_IDS.has(c.id)); const partiallySupportedClients = clients.filter((c) => PARTIALLY_SUPPORTED_IDS.has(c.id)); @@ -401,7 +405,7 @@ export default function AppsStep({
We currently only support local MCP servers, not Connectors. You should remove your connectors manually for your safety and request - a equivalent server in Edison Watch from your admin. + an equivalent server in Edison Watch from your admin.
{partiallySupportedClients.map(renderClientCard)} From c7bea1fce30e3de8f53de9178794490bca50111e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 12:40:42 +0000 Subject: [PATCH 2/6] Make "Edison can't manage this client" a daemon fact, not an app flag Follow-up to the ChatGPT detection commit, from a code-quality review of it. Fixes a real defect that commit introduced, plus one it made permanent. The bug: filtering unmanageable agents inside `applyIntegrations` guarded one caller of two. `bootstrap.ts` sends the saved app selection straight to `enroll` on every start, using its own private dash-to-underscore helper, and never saw the filter. The selection is additive daemon-side (only `unenroll` removes), so a default onboarding run on a Mac with ChatGPT.app put `chatgpt` in `selected_agents` for good. The previous commit message and docblock both claimed this could not happen. `heal_edison_install` then counted and logged a self-heal for it on every reconcile pass - every fs event plus every 20s tick - because the log and `healed += 1` sat outside the loop over `edison_installs()`. So the self-heal signal was permanently non-zero and a genuine heal became indistinguishable from background noise. That was already latent for JetBrains with no IDE installed; ChatGPT made it certain. Both are fixed at the layer that owns the fact: - `Agent::is_manageable()`, declared (not inferred from an empty `edison_installs`, which conflates "no target right now" with "never has one" - the JetBrains case). `enroll` and `apply_integrations` filter on it, closing both doors with one guard and pruning any stale name a previous build let through. - `heal_edison_install` reports only what it wrote. - `AgentInfo.manageable` carries it to the app, defaulting to true so an older daemon doesn't drop real clients out of setup status. That removes the app's second, hand-maintained copy of the same fact: `connectorOnly` off `ClientDisplay` (whose header promises it mirrors the shared agent-registry, which has no such field), and `MANAGED_CLIENT_LIST` and `isConnectorOnly` deleted. Two user-facing corrections that the app-side flag was hiding: - ChatGPT is reported in ClientsView again, under a new `unmanaged` status ("Not Protected", amber). Excluding it meant the user was warned once during onboarding and never again about an app running unprotected; the alternative of `mcpApplicable: false` would have painted it green. - The wizard no longer renders a checked checkbox for it. Selecting it did nothing, and it inflated the next step's "Configure N Apps" count. `PARTIALLY_SUPPORTED_IDS` is now documented as the presentation grouping it is - Claude Desktop/Cowork are manageable and still belong under that warning - which is the distinction a story comment previously inverted. Tests: dropped two that asserted properties of the language rather than of this code (`Arc` compiles by construction; `[].any()` is false) and covered `default_app_paths`, which is the only function here with real logic and whose failure mode is silent. Added Rust tests for the selection guard and a renderer test for the checkbox, both verified to fail against the unfixed code. Dropped a dead electron mock. The full detectord workspace now builds and tests here after a toolchain update (`libsqlite3-sys` needed a newer stable rustc), so unlike the previous commit the daemon changes are verified rather than inferred: cargo test --workspace, clippy --all-targets --all-features, fmt. Desktop typecheck and 18 vitest files pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LGqPs27HH3TkQ6hQeJ5mAK --- .../crates/edison-detectord/src/agent.rs | 19 ++++ .../edison-detectord/src/clients/chatgpt.rs | 95 ++++++++++++++----- .../crates/mcp_detector_daemon/src/ops.rs | 82 +++++++++++++++- .../mcp_detector_daemon/src/protocol.rs | 9 ++ .../__tests__/connectorOnlyClients.test.ts | 67 ------------- .../src/main/__tests__/hookStatus.test.ts | 1 + .../__tests__/unmanageableClients.test.ts | 83 ++++++++++++++++ .../desktop/src/main/clients/displayMeta.ts | 22 ++--- packages/desktop/src/main/clients/registry.ts | 19 ---- packages/desktop/src/main/detectord/agents.ts | 13 ++- .../src/main/detectord/integrations.ts | 14 +-- .../desktop/src/main/detectord/protocol.ts | 10 ++ packages/desktop/src/main/ipc/ipcHandlers.ts | 11 ++- .../src/main/ipc/ipcHandlersMcpSubmit.ts | 13 +-- .../desktop/src/main/runtime/hookStatus.ts | 24 +++-- packages/desktop/src/preload/index.d.ts | 2 +- packages/desktop/src/preload/index.ts | 2 +- .../src/__tests__/renderSmoke.test.tsx | 29 +++++- .../src/components/main/ClientsView.tsx | 57 ++++++++++- .../onboarding/AppsStep.stories.tsx | 12 ++- .../src/components/onboarding/AppsStep.tsx | 72 +++++++++----- .../src/renderer/src/testing/mockApi.ts | 2 + 22 files changed, 470 insertions(+), 188 deletions(-) delete mode 100644 packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts create mode 100644 packages/desktop/src/main/__tests__/unmanageableClients.test.ts diff --git a/crates/detectord/crates/edison-detectord/src/agent.rs b/crates/detectord/crates/edison-detectord/src/agent.rs index 4ef7ee7..7040075 100644 --- a/crates/detectord/crates/edison-detectord/src/agent.rs +++ b/crates/detectord/crates/edison-detectord/src/agent.rs @@ -26,6 +26,25 @@ pub trait Agent: Send + Sync { /// produces no servers. fn is_installed(&self) -> bool; + /// Whether Edison can manage this agent at all: install the `edison-watch` + /// entry, inject hooks, read a config back. False for presence-only agents + /// whose MCP servers live in the vendor's account (ChatGPT's Connectors) + /// rather than in a file on this machine. + /// + /// Declared, not inferred from an empty + /// [`edison_installs`](Agent::edison_installs): "no install target right + /// now" and "never has one" are different facts. JetBrains reports no + /// targets when no IDE is installed and is still perfectly manageable the + /// moment one appears. + /// + /// An unmanageable agent is dropped from the enrolled selection, so nothing + /// downstream tries to install into it or reports it as unconfigured. It is + /// still discovered and still reported as installed - the app's job is to + /// tell the user it is there and outside Edison's reach. + fn is_manageable(&self) -> bool { + true + } + /// Filesystem locations to watch for this agent's MCP config. /// /// A driver subscribes to each [`files`](WatchTargets::files) entry's diff --git a/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs b/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs index b96771f..a779d1b 100644 --- a/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs +++ b/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs @@ -47,6 +47,10 @@ impl Agent for ChatGpt { self.candidates.iter().any(|p| p.exists()) } + fn is_manageable(&self) -> bool { + false + } + fn watch_targets(&self) -> WatchTargets { // No local config exists, so there is no file whose change could mean // "a connector was added". Watching the app bundle would only report @@ -71,10 +75,11 @@ impl Agent for ChatGpt { fn default_app_paths() -> Vec { if cfg!(target_os = "macos") { - // Post-2026 Codex↔ChatGPT merger: the unified Chat + Work + Codex app - // ships as `ChatGPT.app`; the older standalone chat app is - // `ChatGPT Classic.app`. (The Codex *CLI* is a separate, fully - // supported agent — see `clients/codex.rs`.) + // Both bundle names OpenAI has shipped the desktop app under. Probing + // for both is cheap; picking wrong is not, because the failure is + // silent - a user with ChatGPT installed just never sees the warning + // and has nothing to report. (The Codex *CLI* is a separate, fully + // supported agent - see `clients/codex.rs`.) const NAMES: [&str; 2] = ["ChatGPT.app", "ChatGPT Classic.app"]; let mut out: Vec = NAMES .iter() @@ -85,10 +90,11 @@ fn default_app_paths() -> Vec { } out } else if cfg!(target_os = "windows") { - // Shipped through the Microsoft Store (product id 9NT1R1C2HH7J), which - // registers a `ChatGPT.exe` app-execution alias under - // `%LOCALAPPDATA%\Microsoft\WindowsApps`. The `Programs` path covers a - // direct (non-Store) install. + // A Store install registers an app-execution alias under + // `%LOCALAPPDATA%\Microsoft\WindowsApps`; `Programs` covers a direct + // one. The alias is assumed to be named `ChatGPT.exe` (the MSIX + // convention) - unverified against a real Windows install, and the + // one line to change if detection turns out never to fire there. match dirs::data_local_dir() { Some(local) => vec![ local @@ -121,32 +127,75 @@ mod tests { assert!(ChatGpt::from_paths(vec![app, missing]).is_installed()); } - #[test] - fn is_usable_as_a_shared_trait_object() { - // How the daemon holds it: `Vec>`, shared across the - // watcher's threads. Compile-time check that nothing here broke `Send`. - let agent: std::sync::Arc = - std::sync::Arc::new(ChatGpt::discover().expect("infallible")); - assert_eq!(agent.name(), CLIENT_NAME); - } - - #[test] - fn never_installed_without_candidates() { - // The Linux case: no official desktop app, so no paths to probe. - assert!(!ChatGpt::from_paths(Vec::new()).is_installed()); - } - #[test] fn discovers_nothing_and_is_not_an_install_target() { + // The null-object contract the whole app-side design rests on: ChatGPT + // is reported as present and nothing else. If any of these ever returns + // something, the app has to stop calling it unmanageable. let dir = tempdir().unwrap(); let app = dir.path().join("ChatGPT.app"); std::fs::create_dir(&app).unwrap(); let agent = ChatGpt::from_paths(vec![app]); assert!(agent.is_installed()); + assert!(!agent.is_manageable()); assert!(agent.discover().unwrap().is_empty()); assert!(agent.edison_installs(dir.path()).is_empty()); assert!(agent.hook_install(dir.path()).is_none()); assert!(agent.watch_targets().files.is_empty()); } + + // `default_app_paths` is the only part of this file with real logic, and + // the only way it can fail is silently: probe the wrong place and ChatGPT + // is simply never detected, which no user reports because all they see is + // the absence of a warning. The platform it runs on is the platform under + // test - these run in CI on all three. + + #[test] + #[cfg(target_os = "macos")] + fn macos_probes_both_bundles_in_both_application_dirs() { + let paths = default_app_paths(); + let ends_with = |name: &str| { + paths + .iter() + .filter(|p| p.file_name().is_some_and(|f| f == name)) + .count() + }; + // Both names, under /Applications and ~/Applications. + assert_eq!(ends_with("ChatGPT.app"), 2); + assert_eq!(ends_with("ChatGPT Classic.app"), 2); + assert!(paths.iter().any(|p| p.starts_with("/Applications"))); + let home = dirs::home_dir().expect("a home dir"); + assert!( + paths + .iter() + .any(|p| p.starts_with(home.join("Applications"))) + ); + } + + #[test] + #[cfg(target_os = "windows")] + fn windows_probes_the_store_alias_and_a_direct_install() { + let paths = default_app_paths(); + assert!( + paths + .iter() + .any(|p| p.ends_with("Microsoft\\WindowsApps\\ChatGPT.exe")) + ); + assert!( + paths + .iter() + .any(|p| p.ends_with("Programs\\ChatGPT\\ChatGPT.exe")) + ); + } + + #[test] + #[cfg(not(any(target_os = "macos", target_os = "windows")))] + fn linux_probes_nothing_so_chatgpt_is_never_reported() { + // There is no official Linux desktop app. Detecting one would put an + // unremovable "partially supported" warning in front of a user who + // cannot possibly have it installed. + assert!(default_app_paths().is_empty()); + assert!(!ChatGpt::discover().unwrap().is_installed()); + } } diff --git a/crates/detectord/crates/mcp_detector_daemon/src/ops.rs b/crates/detectord/crates/mcp_detector_daemon/src/ops.rs index 83f8b6d..a1f2cea 100644 --- a/crates/detectord/crates/mcp_detector_daemon/src/ops.rs +++ b/crates/detectord/crates/mcp_detector_daemon/src/ops.rs @@ -18,6 +18,27 @@ use crate::platform; use crate::protocol::{AgentInfo, Choice, IntegrationChange, ServerView, Status}; use crate::quarantined::{QuarantinedEntry, QuarantinedState}; +/// Drop agent names Edison cannot manage (ChatGPT and any future host whose +/// MCP servers live in the vendor's account) from a selection list. +/// +/// An unknown name is kept: it is most likely an agent this build doesn't +/// compile in, and silently dropping it would erase a selection that a fuller +/// build understands. +fn retain_manageable(agents: &mut Vec) { + let unmanageable: Vec<&'static str> = agents::build() + .iter() + .filter(|a| !a.is_manageable()) + .map(|a| a.name()) + .collect(); + agents.retain(|name| { + let keep = !unmanageable.iter().any(|u| u == name); + if !keep { + tracing::debug!(agent = %name, "dropping unmanageable agent from selection"); + } + keep + }); +} + /// Which agents are present on the machine, with their workspace hook coverage. /// /// The hook counts are computed here rather than by the UI: the workspace @@ -60,6 +81,7 @@ pub fn list_agents(user: &str) -> Vec { edison_url: installed_edison_entry(a.name(), &installs, &observed) .and_then(|s| edison_entry_url(&s.config)), config_path: installs.first().map(|i| i.path.display().to_string()), + manageable: a.is_manageable(), } }) .collect() @@ -116,20 +138,29 @@ pub fn apply_integrations( user: &str, agents_to_add: &[String], ) -> anyhow::Result> { + // Same guard as `enroll`: an unmanageable agent has nothing to install into, + // and the selection is additive, so letting one in means carrying it for + // good. Dropped up front so it reaches neither the selection nor the + // installer. The app selects every detected app by default, so this is the + // ordinary path, not an edge case. + let mut wanted = agents_to_add.to_vec(); + retain_manageable(&mut wanted); + let mut e = Enrollment::load_for(user)?.ok_or_else(|| anyhow::anyhow!("not enrolled"))?; - for a in agents_to_add { + for a in &wanted { if !e.selected_agents.contains(a) { e.selected_agents.push(a.clone()); } } + retain_manageable(&mut e.selected_agents); e.save_for(user)?; let home = user_home(user); - let changes = install_edison_entries_for(user, &e, &home, agents_to_add); + let changes = install_edison_entries_for(user, &e, &home, &wanted); purge_shadowing_edison_entries(user); // Hooks only for what was asked for. The machine-wide sweep is enroll's job // (`apply_install`), which runs on every app start. - apply_hooks_for(&home, Some(agents_to_add)); + apply_hooks_for(&home, Some(&wanted)); Ok(changes) } @@ -385,7 +416,7 @@ pub async fn enroll( .as_ref() .map(|e| e.selected_agents.clone()) .unwrap_or_default(); - let new_agents = match selected_agents { + let mut new_agents = match selected_agents { Some(provided) => { let mut set = old_agents.clone(); for a in provided { @@ -397,6 +428,12 @@ pub async fn enroll( } None => old_agents, }; + // Selecting an unmanageable agent is meaningless - there is nothing to + // install into - and it does not stay harmless: the selection is additive + // and only `unenroll` removes from it, so one such name would sit in every + // later self-heal pass forever. Filtering the whole union (not just what + // was provided) also prunes any that a previous version let through. + retain_manageable(&mut new_agents); let mcp_base_url = mcp_base_url.or_else(|| existing.as_ref().and_then(|e| e.mcp_base_url.clone())); let edison_secret_key = @@ -703,6 +740,12 @@ pub fn heal_edison_install(user: &str, e: &Enrollment) -> usize { if present.contains(agent.name()) { continue; // already installed — don't rewrite (avoids fs-watch churn) } + // Count and report only what was actually written. An agent with no + // install targets (JetBrains with no IDE on the machine) reaches here + // and writes nothing; logging it as healed anyway made the self-heal + // signal permanently non-zero, so a real heal - somebody's config got + // clobbered - was indistinguishable from the every-20s background hum. + let mut wrote = false; for inst in agent.edison_installs(&home) { let done_via_cli = inst.prefer_cli && { let url = mcp_quarantine::edison_url(mcp_base, &e.api_key, &inst.client_id); @@ -711,6 +754,10 @@ pub fn heal_edison_install(user: &str, e: &Enrollment) -> usize { if !done_via_cli { let _ = mcp_quarantine::install_edison(&inst, mcp_base, &e.api_key, secret); } + wrote = true; + } + if !wrote { + continue; } tracing::info!( agent = agent.name(), @@ -1035,6 +1082,33 @@ fn conflict_detail(err: &BackendError, name: &str) -> String { mod tests { use super::*; + #[test] + fn selection_drops_unmanageable_agents_but_keeps_everything_else() { + // The app sends its saved app selection on every start (enroll) as well + // as on apply, and the selection is additive - only `unenroll` empties + // it. So one unmanageable name getting in is permanent, and it used to + // make every self-heal pass report a heal that never happened. + let mut agents = vec![ + "claude_code".to_string(), + "chatgpt".to_string(), + "cursor".to_string(), + ]; + retain_manageable(&mut agents); + assert_eq!( + agents, + vec!["claude_code".to_string(), "cursor".to_string()] + ); + } + + #[test] + fn selection_keeps_names_this_build_does_not_know() { + // An agent compiled out of this build is not the same as one we refuse + // to manage; dropping it would erase a selection a fuller build honours. + let mut agents = vec!["some_future_agent".to_string()]; + retain_manageable(&mut agents); + assert_eq!(agents, vec!["some_future_agent".to_string()]); + } + fn quarantine_record(path: &str) -> QuarantineRecord { QuarantineRecord { kind: SourceKind::Json, diff --git a/crates/detectord/crates/mcp_detector_daemon/src/protocol.rs b/crates/detectord/crates/mcp_detector_daemon/src/protocol.rs index eee053e..515c99c 100644 --- a/crates/detectord/crates/mcp_detector_daemon/src/protocol.rs +++ b/crates/detectord/crates/mcp_detector_daemon/src/protocol.rs @@ -219,6 +219,15 @@ pub struct AgentInfo { pub workspace_hooks_total: u32, #[serde(default)] pub workspace_hooks_installed: u32, + /// Whether Edison can manage this agent at all, or only report that it is + /// there. False for hosts whose MCP servers are Connectors in the vendor's + /// account (ChatGPT), where there is no local config to read or write. + /// + /// Defaults to `true` — what every agent predating this field is. An older + /// daemon omitting it must not make the app treat real clients as + /// unmanageable and quietly stop reporting their setup status. + #[serde(default = "default_true")] + pub manageable: bool, } /// One discovered server instance (not deduped — carries its source path). diff --git a/packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts b/packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts deleted file mode 100644 index 286a87d..0000000 --- a/packages/desktop/src/main/__tests__/connectorOnlyClients.test.ts +++ /dev/null @@ -1,67 +0,0 @@ -import { describe, it, expect, beforeEach, vi } from 'vitest' - -/** - * ChatGPT is a detect-only client: its MCP servers are Connectors hosted in the - * user's OpenAI account, so there is no local config for Edison to read, write, - * hook, or proxy. The whole "surface ChatGPT in the wizard without pretending we - * protect it" design rests on that staying true, so it's pinned here - if - * ChatGPT is ever given a real config surface, these are the tests to revisit. - */ - -const applyIntegrationsRpc = vi.fn(() => Promise.resolve([])) -const connect = vi.fn(() => Promise.resolve()) - -vi.mock('../detectord/lifecycle', () => ({ - getDetectordClient: () => ({ connect, applyIntegrations: applyIntegrationsRpc }) -})) - -// health drags in electron (dialogs for the missing-binary case). -vi.mock('../detectord/health', () => ({ - withDetectordHealth: (_label: string, fn: () => Promise) => fn() -})) - -vi.mock('electron', () => ({ app: { getPath: () => '/tmp' }, BrowserWindow: { getAllWindows: () => [] } })) - -import { applyIntegrations } from '../detectord/integrations' -import { CLIENT_DISPLAY } from '../clients/displayMeta' -import { CLIENT_LIST, MANAGED_CLIENT_LIST, isConnectorOnly } from '../clients/registry' - -describe('connector-only clients', () => { - beforeEach(() => { - applyIntegrationsRpc.mockClear() - connect.mockClear() - }) - - it('describes ChatGPT as connector-only, with a label instead of a path', () => { - expect(isConnectorOnly('chatgpt')).toBe(true) - // The wizard renders this where a config path would go; blank would read as - // "we couldn't find it" rather than "there is nothing to find". - expect(CLIENT_DISPLAY.chatgpt.configLabel).toBeTruthy() - }) - - it('treats every other client as manageable', () => { - expect(isConnectorOnly('claude-code')).toBe(false) - // Claude Desktop/Cowork are "partially supported" in the wizard but DO have - // a local config Edison writes, so they stay manageable. - expect(isConnectorOnly('claude-desktop')).toBe(false) - expect(isConnectorOnly('claude-cowork')).toBe(false) - }) - - it('leaves ChatGPT out of the managed list, so it gets no setup status', () => { - expect(CLIENT_LIST.map((c) => c.id)).toContain('chatgpt') - expect(MANAGED_CLIENT_LIST.map((c) => c.id)).not.toContain('chatgpt') - }) - - it('never asks the daemon to install into ChatGPT', async () => { - // The wizard selects every detected app by default, so this is the ordinary - // case, not an edge one. - expect(await applyIntegrations(['chatgpt'])).toEqual([]) - expect(connect).not.toHaveBeenCalled() - expect(applyIntegrationsRpc).not.toHaveBeenCalled() - }) - - it('still installs the other apps selected alongside it', async () => { - await applyIntegrations(['claude-code', 'chatgpt', 'cursor']) - expect(applyIntegrationsRpc).toHaveBeenCalledWith(['claude_code', 'cursor']) - }) -}) diff --git a/packages/desktop/src/main/__tests__/hookStatus.test.ts b/packages/desktop/src/main/__tests__/hookStatus.test.ts index 90ac4f0..0b947e1 100644 --- a/packages/desktop/src/main/__tests__/hookStatus.test.ts +++ b/packages/desktop/src/main/__tests__/hookStatus.test.ts @@ -31,6 +31,7 @@ function agent(over: Partial = {}): AgentFacts { workspaceHooksTotal: 0, edisonUrl: EXPECTED_URL, configPath: '/home/u/.claude.json', + manageable: true, ...over } } diff --git a/packages/desktop/src/main/__tests__/unmanageableClients.test.ts b/packages/desktop/src/main/__tests__/unmanageableClients.test.ts new file mode 100644 index 0000000..1ba0083 --- /dev/null +++ b/packages/desktop/src/main/__tests__/unmanageableClients.test.ts @@ -0,0 +1,83 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +import type { AgentFacts } from '../detectord/agents' +import type { McpClientId } from '../discovery/types' + +/** + * ChatGPT is presence-only: its MCP servers are Connectors in the user's OpenAI + * account, so there is nothing local to read, write, hook, or proxy. Two things + * have to hold, and both have been wrong at some point: + * + * - it never enters the enrolled selection (every path, not just the obvious + * one), because the selection is additive and only `unenroll` empties it; + * - it is still REPORTED, with a status of its own. Dropping it from the + * client list is how a user ends up assuming an unprotected app is covered. + */ + +let facts: Map | null = new Map() + +vi.mock('../detectord/agents', async (importOriginal) => ({ + ...(await importOriginal()), + getAgentFacts: () => Promise.resolve(facts) +})) + +// hookStatus reaches DetectordUnavailableError through mcpDiscovery, whose +// import graph ends up in electron. +vi.mock('electron', () => ({ app: { getPath: () => '/tmp' }, BrowserWindow: { getAllWindows: () => [] } })) + +import { getHookStatus } from '../runtime/hookStatus' + +const EXPECTED_URL = 'https://mcp.edison.watch/mcp' + +function agent(over: Partial = {}): AgentFacts { + return { + installed: true, + hooksInstalled: 0, + hooksTotal: 0, + workspaceHooksInstalled: 0, + workspaceHooksTotal: 0, + edisonUrl: null, + configPath: null, + manageable: true, + ...over + } +} + +describe('unmanageable clients', () => { + beforeEach(() => { + facts = new Map([['chatgpt' as McpClientId, agent({ manageable: false })]]) + }) + + it('reports ChatGPT rather than hiding it', async () => { + const all = await getHookStatus(EXPECTED_URL, true) + const entry = all.find((s) => s.client === 'chatgpt') + expect(entry).toBeDefined() + expect(entry?.installed).toBe(true) + }) + + it('marks it unmanageable and scores it against no setup conditions', async () => { + // Both would otherwise render as a lie: an unmet MCP condition reads + // "gateway not configured" (unfixable), and a met one paints it green. + const entry = (await getHookStatus(EXPECTED_URL, true)).find((s) => s.client === 'chatgpt') + expect(entry?.manageable).toBe(false) + expect(entry?.mcpApplicable).toBe(false) + expect(entry?.hooksApplicable).toBe(false) + }) + + it('leaves manageable clients scored as before', async () => { + facts = new Map([ + ['cursor' as McpClientId, agent({ edisonUrl: EXPECTED_URL, hooksTotal: 4, hooksInstalled: 4 })] + ]) + const entry = (await getHookStatus(EXPECTED_URL, true)).find((s) => s.client === 'cursor') + expect(entry?.manageable).toBe(true) + expect(entry?.mcpApplicable).toBe(true) + expect(entry?.mcpConnected).toBe(true) + }) + + it('treats a daemon that never heard of the field as fully manageable', async () => { + // An older daemon omits `manageable`; defaulting it to false would silently + // drop every real client out of setup reporting. + const { UNKNOWN_AGENT_FACTS } = await import('../detectord/agents') + expect(UNKNOWN_AGENT_FACTS.manageable).toBe(true) + }) +}) diff --git a/packages/desktop/src/main/clients/displayMeta.ts b/packages/desktop/src/main/clients/displayMeta.ts index ee2dfbb..5368723 100644 --- a/packages/desktop/src/main/clients/displayMeta.ts +++ b/packages/desktop/src/main/clients/displayMeta.ts @@ -1,7 +1,8 @@ /** * Display metadata (name + brand color) for every supported client. * - * Mirrors the entries in `@edison-watch/shared/agent-registry`. Duplicated here so + * `name` and `brandColor` mirror `@edison-watch/shared/agent-registry`; `configLabel` + * is app-local copy with no counterpart there. Duplicated here so * main-process code can build ClientIntegration objects without dragging the * shared package into test module graphs (vitest can't resolve subpath * exports of an unbuilt package). Keep in sync with the shared registry. @@ -12,17 +13,13 @@ export interface ClientDisplay { name: string brandColor: string /** - * The client keeps its MCP servers as hosted Connectors in the user's account - * rather than in a local config file. Edison can see the app is installed but - * has nothing to read, write, hook, or proxy - so these clients are detected - * and flagged, never managed. - */ - connectorOnly?: boolean - /** - * What to show where a config path would go, for `connectorOnly` clients. - * The daemon reports no path for them (there isn't one), and a blank line - * under the app name reads as "we couldn't find it" rather than "there is - * nothing to find". + * What to show where a config path would go, for clients that have none. + * The daemon reports a null path for them (there is no file), and a blank + * line under the app name reads as "we couldn't find it" rather than "there + * is nothing to find". + * + * Display copy only - whether Edison can manage a client is the daemon's + * `manageable`, never the presence of this string. */ configLabel?: string } @@ -42,7 +39,6 @@ export const CLIENT_DISPLAY: Record = { chatgpt: { name: 'ChatGPT', brandColor: '#000000', - connectorOnly: true, configLabel: 'Connectors · managed server-side in your ChatGPT account', }, } diff --git a/packages/desktop/src/main/clients/registry.ts b/packages/desktop/src/main/clients/registry.ts index c6cdc3c..6f4e47b 100644 --- a/packages/desktop/src/main/clients/registry.ts +++ b/packages/desktop/src/main/clients/registry.ts @@ -20,22 +20,3 @@ export const CLIENT_LIST: ClientEntry[] = ( ).map((id) => ({ id, display: CLIENT_DISPLAY[id] })) export const CLIENT_IDS: McpClientId[] = CLIENT_LIST.map((c) => c.id) - -/** - * The clients Edison can actually manage - i.e. everything except the - * connector-only ones (ChatGPT), whose MCP servers live in the user's account. - * - * Setup status is reported over this list rather than `CLIENT_LIST`, because - * every answer it could give for a connector-only client is wrong: "gateway not - * configured" blames the user for something they can't fix, and "nothing - * applicable, all good" paints an unprotected app green. It gets detected and - * flagged in the onboarding wizard instead. - */ -export const MANAGED_CLIENT_LIST: ClientEntry[] = CLIENT_LIST.filter( - (c) => !c.display.connectorOnly -) - -/** Whether this client is detect-only (server-side Connectors, no local config). */ -export function isConnectorOnly(clientId: string): boolean { - return CLIENT_DISPLAY[clientId as McpClientId]?.connectorOnly === true -} diff --git a/packages/desktop/src/main/detectord/agents.ts b/packages/desktop/src/main/detectord/agents.ts index a2b8c37..105f4b4 100644 --- a/packages/desktop/src/main/detectord/agents.ts +++ b/packages/desktop/src/main/detectord/agents.ts @@ -29,6 +29,12 @@ export interface AgentFacts { edisonUrl: string | null /** The agent's user-scope config file, when it has one. */ configPath: string | null + /** + * Whether Edison can manage this agent, or only report that it's installed. + * False for connector-only hosts (ChatGPT), whose MCP servers live in the + * vendor's account rather than in a file on this machine. + */ + manageable: boolean } const UNKNOWN: AgentFacts = { @@ -38,7 +44,8 @@ const UNKNOWN: AgentFacts = { workspaceHooksInstalled: 0, workspaceHooksTotal: 0, edisonUrl: null, - configPath: null + configPath: null, + manageable: true } function toFacts(a: AgentInfo): AgentFacts { @@ -49,7 +56,9 @@ function toFacts(a: AgentInfo): AgentFacts { workspaceHooksInstalled: a.workspace_hooks_installed ?? 0, workspaceHooksTotal: a.workspace_hooks_total ?? 0, edisonUrl: a.edison_url ?? null, - configPath: a.config_path ?? null + configPath: a.config_path ?? null, + // Absent means an older daemon, where every agent was manageable. + manageable: a.manageable ?? true } } diff --git a/packages/desktop/src/main/detectord/integrations.ts b/packages/desktop/src/main/detectord/integrations.ts index 0409b3b..097b4e3 100644 --- a/packages/desktop/src/main/detectord/integrations.ts +++ b/packages/desktop/src/main/detectord/integrations.ts @@ -9,7 +9,6 @@ import { getDetectordClient } from './lifecycle' import { toAgentName } from './agents' import { withDetectordHealth } from './health' -import { isConnectorOnly } from '../clients/registry' import type { IntegrationChange } from './protocol' export type { IntegrationChange } @@ -17,20 +16,15 @@ export type { IntegrationChange } /** * Install the edison-watch entry + hooks for these client ids. * - * Connector-only clients (ChatGPT) are dropped first. The wizard selects every - * detected app by default, so they arrive here routinely - and asking the - * daemon to install into one would add it to the enrolled selection, which the - * self-heal then revisits forever, all to write a config file that does not - * exist. Silently skipping matches what the user is told about them: detected, - * not managed. + * Unmanageable clients are not filtered here. The daemon drops them, because + * it is not the only caller: enroll sends the saved app selection on every + * start, and a guard in front of one caller left the other wide open. */ export async function applyIntegrations(clients: string[]): Promise { - const installable = clients.filter((c) => !isConnectorOnly(c)) - if (installable.length === 0) return [] return withDetectordHealth('apply_integrations', async () => { const daemon = getDetectordClient() await daemon.connect() - return daemon.applyIntegrations(installable.map(toAgentName)) + return daemon.applyIntegrations(clients.map(toAgentName)) }) } diff --git a/packages/desktop/src/main/detectord/protocol.ts b/packages/desktop/src/main/detectord/protocol.ts index df71b14..d77a519 100644 --- a/packages/desktop/src/main/detectord/protocol.ts +++ b/packages/desktop/src/main/detectord/protocol.ts @@ -81,6 +81,16 @@ export interface AgentInfo { */ workspace_hooks_total?: number workspace_hooks_installed?: number + /** + * Whether Edison can manage this agent at all, or only report that it's + * there. False for hosts whose MCP servers are Connectors in the vendor's + * account (ChatGPT) - nothing local to read, write, hook, or proxy. + * + * Absent from daemons predating the field, and absence must read as `true`: + * every agent that existed before it is manageable, and defaulting the other + * way would silently drop real clients out of setup status. + */ + manageable?: boolean } /** One discovered server instance. `state`: edison | known | new | opaque | report. */ diff --git a/packages/desktop/src/main/ipc/ipcHandlers.ts b/packages/desktop/src/main/ipc/ipcHandlers.ts index 2fff743..433ee0c 100644 --- a/packages/desktop/src/main/ipc/ipcHandlers.ts +++ b/packages/desktop/src/main/ipc/ipcHandlers.ts @@ -409,15 +409,18 @@ export function registerIpcHandlers(deps: IpcHandlerDeps): void { // Distinguish "no agents installed" from "nobody answered": the renderer // shows the daemon warning for the latter instead of an empty app list. if (!facts) return { clients: [], daemonUnavailable: true } - const clients: Array<{ id: string; name: string; configPath: string }> = [] + const clients: Array<{ id: string; name: string; configPath: string; manageable: boolean }> = [] for (const [id, f] of facts) { if (!f.installed) continue clients.push({ id, name: CLIENT_DISPLAY[id]?.name ?? id, - // Connector-only clients have no path to report - they get an advisory - // label saying where their MCP config actually lives instead. - configPath: f.configPath ?? CLIENT_DISPLAY[id]?.configLabel ?? '' + // A client with no local config gets an advisory label in place of the + // path, saying where its MCP config actually lives. + configPath: f.configPath ?? CLIENT_DISPLAY[id]?.configLabel ?? '', + // Drives whether the wizard offers a checkbox at all: selecting a + // client Edison can't configure does nothing. + manageable: f.manageable }) } return { clients, daemonUnavailable: false } diff --git a/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts b/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts index 236380c..c7a7959 100644 --- a/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts +++ b/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts @@ -14,7 +14,7 @@ import { submitOneViaDetectord, } from "../detectord/submit"; import { getDetectordClient } from "../detectord/lifecycle"; -import { toAgentName } from "../detectord/agents"; +import { toAgentName, getAgentFacts } from "../detectord/agents"; import { applyIntegrations, revertIntegrations, integrationErrors } from "../detectord/integrations"; import { CLIENT_DISPLAY } from "../clients/displayMeta"; import { detectSecrets } from "../discovery/secretDetection"; @@ -137,14 +137,15 @@ export function registerMcpSubmitHandlers(): void { // Show an agent's config file. The daemon reads it: it owns agent files, and // this used to take an arbitrary path from the renderer. ipcMain.handle("mcp:readConfig", async (_event, client: string) => { - // A connector-only client has no local config, so asking the daemon for one + // An unmanageable client has no local config, so asking the daemon for one // only produces an error the user can do nothing about. Say what's actually - // going on instead. - const display = CLIENT_DISPLAY[client as McpClientId]; - if (display?.connectorOnly) { + // going on instead. The daemon is the authority on which those are. + const facts = await getAgentFacts(); + if (facts?.get(client as McpClientId)?.manageable === false) { + const name = CLIENT_DISPLAY[client as McpClientId]?.name ?? client; return { content: - `${display.name} keeps its MCP servers as Connectors in your account, not in a ` + + `${name} keeps its MCP servers as Connectors in your account, not in a ` + `local config file. There is nothing here for Edison Watch to read or protect.`, }; } diff --git a/packages/desktop/src/main/runtime/hookStatus.ts b/packages/desktop/src/main/runtime/hookStatus.ts index 1552e61..9b70af1 100644 --- a/packages/desktop/src/main/runtime/hookStatus.ts +++ b/packages/desktop/src/main/runtime/hookStatus.ts @@ -10,7 +10,7 @@ import { getAgentFacts, type AgentFacts } from '../detectord/agents' import { DetectordUnavailableError } from '../discovery/mcpDiscovery' -import { MANAGED_CLIENT_LIST } from '../clients/registry' +import { CLIENT_LIST } from '../clients/registry' import type { McpClientId } from '../discovery/types' import type { ClaudeCodeMcpStatus } from '../infra/setupConfig' @@ -24,6 +24,13 @@ export interface HookStatusEntry { mcpConfigured: boolean mcpApplicable: boolean hooksApplicable: boolean + /** + * False when Edison can only see this client, not configure it (ChatGPT's + * server-side Connectors). Reported rather than filtered out: the wizard is + * seen once, this list is the permanent surface, and a client silently + * missing from it is how a user ends up assuming they're covered. + */ + manageable: boolean mcpRuntimeStatus?: ClaudeCodeMcpStatus } @@ -61,9 +68,7 @@ export async function getHookStatus( // would read as a broken installation. Say we don't know instead. if (!facts) throw new DetectordUnavailableError() - // Connector-only clients are excluded: there is no hook surface and no - // gateway entry to install, so any status line for them misleads. - return MANAGED_CLIENT_LIST.map((client) => { + return CLIENT_LIST.map((client) => { const f = facts.get(client.id) if (!f) { // The daemon didn't report this agent (unreachable, or too old to know @@ -77,7 +82,8 @@ export async function getHookStatus( mcpConnected: false, mcpConfigured: false, mcpApplicable: true, - hooksApplicable: false + hooksApplicable: false, + manageable: true } } @@ -108,8 +114,12 @@ export async function getHookStatus( totalHooks, mcpConnected, mcpConfigured, - mcpApplicable: true, - hooksApplicable: totalHooks > 0, + // An unmanageable client has no gateway entry and no hook surface, so + // neither condition applies to it. The UI renders it as its own state + // rather than scoring it against conditions it can never meet. + mcpApplicable: f.manageable, + hooksApplicable: f.manageable && totalHooks > 0, + manageable: f.manageable, ...(mcpRuntimeStatus !== undefined ? { mcpRuntimeStatus } : {}) } }) diff --git a/packages/desktop/src/preload/index.d.ts b/packages/desktop/src/preload/index.d.ts index b08d2ef..5149daf 100644 --- a/packages/desktop/src/preload/index.d.ts +++ b/packages/desktop/src/preload/index.d.ts @@ -34,7 +34,7 @@ interface EdisonAPI { } mcp: { detectClients: () => Promise<{ - clients: Array<{ id: string; name: string; configPath: string }> + clients: Array<{ id: string; name: string; configPath: string; manageable: boolean }> daemonUnavailable: boolean }> discover: () => Promise<{ diff --git a/packages/desktop/src/preload/index.ts b/packages/desktop/src/preload/index.ts index ecaae07..20d72d6 100644 --- a/packages/desktop/src/preload/index.ts +++ b/packages/desktop/src/preload/index.ts @@ -66,7 +66,7 @@ const api = { /** MCP client discovery and hook management */ mcp: { detectClients: (): Promise<{ - clients: Array<{ id: string; name: string; configPath: string }> + clients: Array<{ id: string; name: string; configPath: string; manageable: boolean }> daemonUnavailable: boolean }> => ipcRenderer.invoke('mcp:detectClients'), diff --git a/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx b/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx index 1b6d04c..d7aadef 100644 --- a/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx +++ b/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx @@ -52,7 +52,9 @@ describe('AppsStep', () => { it('renders the clients the daemon reports', async () => { const api = installMockApi() ;(api.mcp as Record).detectClients = async () => ({ - clients: [{ id: 'cursor', name: 'Cursor', configPath: '/home/u/.cursor/mcp.json' }], + clients: [ + { id: 'cursor', name: 'Cursor', configPath: '/home/u/.cursor/mcp.json', manageable: true } + ], daemonUnavailable: false }) @@ -62,6 +64,31 @@ describe('AppsStep', () => { expectNoRenderErrors() }) + it('offers no selection for a client it cannot configure', async () => { + // A checked checkbox whose value is thrown away downstream told the user + // ChatGPT was about to be configured, and inflated the "Configure N Apps" + // count on the next step by one. + const api = installMockApi() + ;(api.mcp as Record).detectClients = async () => ({ + clients: [ + { id: 'cursor', name: 'Cursor', configPath: '/home/u/.cursor/mcp.json', manageable: true }, + { id: 'chatgpt', name: 'ChatGPT', configPath: 'Connectors', manageable: false } + ], + daemonUnavailable: false + }) + + render( {}} />) + + // Shown, and shown as unprotected - not quietly dropped from the list. + expect(await screen.findByText('ChatGPT')).toBeTruthy() + expect(screen.getByText(/not protected/i)).toBeTruthy() + // Only Cursor is selectable, so only Cursor is counted. + await waitFor(() => { + expect(screen.getByText(/Continue with 1 App$/)).toBeTruthy() + }) + expectNoRenderErrors() + }) + it('says the daemon is unreachable instead of "no clients" during an outage', async () => { const api = installMockApi() const mcp = api.mcp as Record diff --git a/packages/desktop/src/renderer/src/components/main/ClientsView.tsx b/packages/desktop/src/renderer/src/components/main/ClientsView.tsx index d41f5e5..b6798d0 100644 --- a/packages/desktop/src/renderer/src/components/main/ClientsView.tsx +++ b/packages/desktop/src/renderer/src/components/main/ClientsView.tsx @@ -16,6 +16,8 @@ interface HookStatus { mcpConfigured: boolean; mcpApplicable: boolean; hooksApplicable: boolean; + /** False when Edison can only see this client, not configure it. */ + manageable: boolean; mcpRuntimeStatus?: ClaudeCodeMcpStatus; } @@ -30,12 +32,18 @@ interface ClientInfo { mcpConfigured: boolean; mcpApplicable: boolean; hooksApplicable: boolean; + /** False when Edison can only see this client, not configure it. */ + manageable: boolean; mcpRuntimeStatus?: ClaudeCodeMcpStatus; } -// Map client IDs (from McpClientId) to display names +// Map client IDs (from McpClientId) to display names. Every id the daemon can +// report needs an entry - a missing one renders as the raw id. const CLIENT_NAMES: Record = { "claude-code": "Claude Code", + "claude-desktop": "Claude Desktop", + "claude-cowork": "Claude Cowork", + chatgpt: "ChatGPT", cursor: "Cursor", windsurf: "Windsurf (early alpha)", codex: "Codex", @@ -46,12 +54,21 @@ const CLIENT_NAMES: Record = { webstorm: "WebStorm", }; -type ClientStatus = "connected" | "partial-setup" | "installed" | "missing"; +/** + * `unmanaged` is installed-and-outside-our-reach: a client whose MCP servers + * are Connectors in the vendor's account, so there is nothing for Edison to + * configure. It exists because the alternatives both lie - scoring such a + * client against setup conditions reports "gateway not configured" (blaming + * the user for something they cannot fix), and dropping it from the list tells + * them nothing at all about an app that is running unprotected. + */ +type ClientStatus = "connected" | "partial-setup" | "installed" | "unmanaged" | "missing"; function StatusDot({ status }: { status: ClientStatus }) { const colors: Record = { connected: "bg-emerald-400", "partial-setup": "bg-amber-400", + unmanaged: "bg-amber-400", installed: "bg-red-400", missing: "bg-gray-500", }; @@ -69,6 +86,9 @@ function StatusDot({ status }: { status: ClientStatus }) { function getClientStatus(client: ClientInfo): ClientStatus { if (!client.installed) return "missing"; + // Before the setup conditions, because none of them apply: there is no + // gateway entry to install and no hook surface to inject. + if (!client.manageable) return "unmanaged"; const needsMcp = client.mcpApplicable; const needsHooks = client.hooksApplicable; const hooksSatisfied = !needsHooks || client.hasHook; @@ -109,6 +129,23 @@ function getIssueDetail(client: ClientInfo): string { /** Tooltip showing connection condition checklist on hover. */ function ConditionTooltip({ client }: { client: ClientInfo }) { + if (!client.manageable) { + return ( +
+
+

+ Not protected +

+

+ This app's MCP servers are Connectors held in your account, not + local config Edison Watch can proxy. Remove them and request + equivalents from your admin. +

+
+
+ ); + } + const conditions = [ { label: "Installed", met: client.installed }, ...(client.hooksApplicable @@ -176,6 +213,7 @@ export default function ClientsView(): React.ReactNode { mcpConfigured: s.mcpConfigured ?? false, mcpApplicable: s.mcpApplicable ?? true, hooksApplicable: s.hooksApplicable ?? true, + manageable: s.manageable ?? true, mcpRuntimeStatus: s.mcpRuntimeStatus, })), ); @@ -233,6 +271,7 @@ export default function ClientsView(): React.ReactNode { const connected = clients.filter((c) => getClientStatus(c) === "connected"); const partialSetup = clients.filter((c) => getClientStatus(c) === "partial-setup"); const noSetup = clients.filter((c) => getClientStatus(c) === "installed"); + const unmanaged = clients.filter((c) => getClientStatus(c) === "unmanaged"); const notInstalled = clients.filter((c) => getClientStatus(c) === "missing"); return ( @@ -270,6 +309,9 @@ export default function ClientsView(): React.ReactNode { { status: "installed" as ClientStatus, items: noSetup, label: "not set up", bg: "bg-red-500/10", text: "text-red-400", activeBorder: "border-red-500/40 ring-1 ring-red-500/20", show: noSetup.length > 0 }, + { status: "unmanaged" as ClientStatus, items: unmanaged, label: "not protected", + bg: "bg-amber-500/10", text: "text-amber-400", + activeBorder: "border-amber-500/40 ring-1 ring-amber-500/20", show: unmanaged.length > 0 }, { status: "missing" as ClientStatus, items: notInstalled, label: "not found", bg: "bg-gray-500/10", text: "text-gray-400", activeBorder: "border-gray-500/40 ring-1 ring-gray-500/20", show: notInstalled.length > 0 }, @@ -298,6 +340,7 @@ export default function ClientsView(): React.ReactNode { connected: "border-emerald-500/20", "partial-setup": "border-amber-500/20", installed: "border-red-500/20", + unmanaged: "border-amber-500/20", missing: "border-gray-500/20", }; return ( @@ -326,6 +369,7 @@ export default function ClientsView(): React.ReactNode { connected: "Connected", "partial-setup": "Incomplete", installed: "Not Set Up", + unmanaged: "Not Protected", missing: "Not Installed", }; @@ -333,6 +377,7 @@ export default function ClientsView(): React.ReactNode { connected: "success", "partial-setup": "warning", installed: "danger", + unmanaged: "warning", missing: "neutral", }; @@ -340,10 +385,16 @@ export default function ClientsView(): React.ReactNode { connected: "border-emerald-500/20 bg-emerald-500/5", "partial-setup": "border-amber-500/15 bg-amber-500/5", installed: "border-red-500/15 bg-red-500/5", + unmanaged: "border-amber-500/15 bg-amber-500/5", missing: "border-[var(--border)] bg-[var(--bg-raised)] opacity-60", }; - const issueDetail = status === "partial-setup" ? getIssueDetail(client) : null; + const issueDetail = + status === "partial-setup" + ? getIssueDetail(client) + : status === "unmanaged" + ? "Connectors are managed in your account - Edison Watch can't proxy them" + : null; return (
{ setClients((prev) => - prev.map((c) => (c.id === id ? { ...c, enabled: !c.enabled } : c)), + prev.map((c) => (c.id === id && c.manageable ? { ...c, enabled: !c.enabled } : c)), ); }; @@ -273,10 +282,12 @@ export default function AppsStep({ const selectedCount = clients.filter((c) => c.enabled).length; - // Clients whose MCP config lives server-side as hosted Connectors (Claude - // account / OpenAI account), not in a local file Edison can proxy. We can - // detect the app but not protect its connectors, so we surface the warning - // below and ask the user to remove connectors + request equivalents. + // A presentation grouping, NOT a capability: these are the clients whose + // users are likely to be running Connectors, so they get the warning banner + // below. Whether Edison can configure a client is `manageable`, which comes + // from the daemon - the two overlap here but are not the same question. + // Claude Desktop/Cowork are manageable (they have a local config Edison + // writes) and still belong under this warning; ChatGPT is neither. const PARTIALLY_SUPPORTED_IDS = new Set(["claude-desktop", "claude-cowork", "chatgpt"]); const fullySupportedClients = clients.filter((c) => !PARTIALLY_SUPPORTED_IDS.has(c.id)); const partiallySupportedClients = clients.filter((c) => PARTIALLY_SUPPORTED_IDS.has(c.id)); @@ -291,26 +302,33 @@ export default function AppsStep({ boxShadow: client.enabled ? "0 0 12px 0 rgba(125, 255, 246, 0.08)" : "none", }} > - {/* Clickable row - toggles selection */} + {/* Row. Toggles selection for clients Edison can configure; for the rest + it is inert text, because a checkbox whose value is discarded is worse + than no checkbox. */}

diff --git a/packages/desktop/src/renderer/src/testing/mockApi.ts b/packages/desktop/src/renderer/src/testing/mockApi.ts index 446a95c..37f1310 100644 --- a/packages/desktop/src/renderer/src/testing/mockApi.ts +++ b/packages/desktop/src/renderer/src/testing/mockApi.ts @@ -22,6 +22,8 @@ export interface MockClient { id: string name: string configPath: string + /** False for hosts Edison can only detect, never configure (ChatGPT). */ + manageable: boolean } type Api = Window['api'] From 8146acc109463fe6ec564b3cc7f21287f560c178 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 12:48:16 +0000 Subject: [PATCH 3/6] Add a ClientsView story covering the unmanaged client state The "Not Protected" state has no other visual coverage: it can only be reached with a daemon that reports `manageable: false`, and the agent that does (ChatGPT) is never installed on Linux, so neither CI nor a sandbox run can produce it from real detection. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LGqPs27HH3TkQ6hQeJ5mAK --- .../components/main/ClientsView.stories.tsx | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx diff --git a/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx b/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx new file mode 100644 index 0000000..f1eeffb --- /dev/null +++ b/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx @@ -0,0 +1,70 @@ +import type { Meta, StoryObj } from '@storybook/react-vite'; +import ClientsView from './ClientsView'; + +const meta: Meta = { + title: 'Client2/ClientsView', + component: ClientsView, + parameters: { layout: 'centered' }, +}; + +export default meta; +type Story = StoryObj; + +/** A status row as the daemon reports it, with the manageable defaults. */ +const status = (over: Record) => ({ + installed: true, + hasHook: true, + hookCount: 4, + totalHooks: 4, + mcpConnected: true, + mcpConfigured: true, + mcpApplicable: true, + hooksApplicable: true, + manageable: true, + ...over, +}); + +/** + * The permanent client surface, including a host Edison can only see. + * + * ChatGPT keeps its MCP servers as Connectors in the user's account, so it is + * reported as `manageable: false` and lands in its own "Not Protected" state - + * neither scored against setup conditions it can never meet, nor dropped from + * the list, which would leave an unprotected app invisible after onboarding. + */ +export const WithAnUnmanageableClient: Story = { + decorators: [ + (Story) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (window as any).api.mcp.getHookStatus = async () => ({ + statuses: [ + status({ client: 'claude-code' }), + status({ client: 'cursor' }), + status({ + client: 'vscode', + hasHook: false, + hookCount: 2, + mcpConnected: false, + }), + status({ + client: 'chatgpt', + manageable: false, + mcpApplicable: false, + hooksApplicable: false, + hasHook: false, + hookCount: 0, + totalHooks: 0, + mcpConnected: false, + mcpConfigured: false, + }), + ], + daemonUnavailable: false, + }); + return ( +

+ +
+ ); + }, + ], +}; From e58a16e2fac013bae112fd2662bf0219f72da263 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 18:39:52 +0000 Subject: [PATCH 4/6] Stop paying for a discovery pass on every config read Three fixes from review on #30. `mcp:readConfig` asked `getAgentFacts()` up front to decide whether a client was manageable, so every successful read paid for a `list_agents` - a full agent-discovery pass plus a workspace hook scan across the user's projects. AppsStep re-reads every expanded client on refresh, so that was one wasted scan per open panel, not one per read. The check now runs only after a read has already failed, which is the only case it can change the answer for. Behaviour is identical; the happy path is one RPC again. `retain_manageable` derived the unmanageable set by rebuilding every agent. `is_manageable()` is declared per type and no filesystem state feeds it, so it cannot change while the process runs - it is now computed once. `apply_integrations` alone was rebuilding the whole agent set twice more per request and re-emitting each constructor's "discover failed" warning along the way. The macOS path test required a home dir that the code it tests treats as optional, so it could fail on a state the code handles correctly. Covers the readConfig handler for the first time: the Connectors message, the passthrough of a real error, and a guard on the extra discovery call. That guard fails against the previous ordering. --- .../edison-detectord/src/clients/chatgpt.rs | 27 +++++--- .../crates/mcp_detector_daemon/src/ops.rs | 24 +++++-- .../__tests__/unmanageableClients.test.ts | 62 ++++++++++++++++++- .../src/main/ipc/ipcHandlersMcpSubmit.ts | 30 +++++---- 4 files changed, 113 insertions(+), 30 deletions(-) diff --git a/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs b/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs index a779d1b..a8bedfc 100644 --- a/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs +++ b/crates/detectord/crates/edison-detectord/src/clients/chatgpt.rs @@ -161,16 +161,25 @@ mod tests { .filter(|p| p.file_name().is_some_and(|f| f == name)) .count() }; - // Both names, under /Applications and ~/Applications. - assert_eq!(ends_with("ChatGPT.app"), 2); - assert_eq!(ends_with("ChatGPT Classic.app"), 2); + // `/Applications` is unconditional; `~/Applications` needs a home dir, + // which the code treats as optional - so the test does too. Asserting + // more than the code promises fails on the code's own valid states. assert!(paths.iter().any(|p| p.starts_with("/Applications"))); - let home = dirs::home_dir().expect("a home dir"); - assert!( - paths - .iter() - .any(|p| p.starts_with(home.join("Applications"))) - ); + match dirs::home_dir() { + Some(home) => { + assert_eq!(ends_with("ChatGPT.app"), 2); + assert_eq!(ends_with("ChatGPT Classic.app"), 2); + assert!( + paths + .iter() + .any(|p| p.starts_with(home.join("Applications"))) + ); + } + None => { + assert_eq!(ends_with("ChatGPT.app"), 1); + assert_eq!(ends_with("ChatGPT Classic.app"), 1); + } + } } #[test] diff --git a/crates/detectord/crates/mcp_detector_daemon/src/ops.rs b/crates/detectord/crates/mcp_detector_daemon/src/ops.rs index a1f2cea..cc54c44 100644 --- a/crates/detectord/crates/mcp_detector_daemon/src/ops.rs +++ b/crates/detectord/crates/mcp_detector_daemon/src/ops.rs @@ -2,6 +2,7 @@ //! user. Returning protocol DTOs keeps the two front-ends in sync. use std::path::PathBuf; +use std::sync::LazyLock; use anyhow::Context; use edison_detectord::{DiscoveredServer, EdisonInstall, ServerConfig, fingerprint}; @@ -18,6 +19,22 @@ use crate::platform; use crate::protocol::{AgentInfo, Choice, IntegrationChange, ServerView, Status}; use crate::quarantined::{QuarantinedEntry, QuarantinedState}; +/// Agent names this build cannot manage, computed once. +/// +/// `is_manageable()` is declared per agent type, so unlike the rest of what +/// `agents::build()` reports it cannot change while the process runs - no +/// filesystem state feeds it. Deriving it on every selection filter meant +/// `apply_integrations` alone rebuilt the whole agent set twice more per +/// request, re-running each constructor's discovery and re-emitting any +/// "discover failed" warning with it. +static UNMANAGEABLE: LazyLock> = LazyLock::new(|| { + agents::build() + .iter() + .filter(|a| !a.is_manageable()) + .map(|a| a.name()) + .collect() +}); + /// Drop agent names Edison cannot manage (ChatGPT and any future host whose /// MCP servers live in the vendor's account) from a selection list. /// @@ -25,13 +42,8 @@ use crate::quarantined::{QuarantinedEntry, QuarantinedState}; /// compile in, and silently dropping it would erase a selection that a fuller /// build understands. fn retain_manageable(agents: &mut Vec) { - let unmanageable: Vec<&'static str> = agents::build() - .iter() - .filter(|a| !a.is_manageable()) - .map(|a| a.name()) - .collect(); agents.retain(|name| { - let keep = !unmanageable.iter().any(|u| u == name); + let keep = !UNMANAGEABLE.iter().any(|u| u == name); if !keep { tracing::debug!(agent = %name, "dropping unmanageable agent from selection"); } diff --git a/packages/desktop/src/main/__tests__/unmanageableClients.test.ts b/packages/desktop/src/main/__tests__/unmanageableClients.test.ts index 1ba0083..3f52497 100644 --- a/packages/desktop/src/main/__tests__/unmanageableClients.test.ts +++ b/packages/desktop/src/main/__tests__/unmanageableClients.test.ts @@ -15,15 +15,31 @@ import type { McpClientId } from '../discovery/types' */ let facts: Map | null = new Map() +/** `list_agents` is a full discovery pass, so who asks for it matters. */ +let factsCalls = 0 vi.mock('../detectord/agents', async (importOriginal) => ({ ...(await importOriginal()), - getAgentFacts: () => Promise.resolve(facts) + getAgentFacts: () => { + factsCalls += 1 + return Promise.resolve(facts) + } })) // hookStatus reaches DetectordUnavailableError through mcpDiscovery, whose -// import graph ends up in electron. -vi.mock('electron', () => ({ app: { getPath: () => '/tmp' }, BrowserWindow: { getAllWindows: () => [] } })) +// import graph ends up in electron. `ipcMain` is a registry the readConfig +// tests below reach into to get at the handler. +const handlers = new Map unknown>() +vi.mock('electron', () => ({ + app: { getPath: () => '/tmp' }, + BrowserWindow: { getAllWindows: () => [] }, + ipcMain: { handle: (channel: string, fn: never) => handlers.set(channel, fn) } +})) + +let readConfigResult: () => Promise<{ content: string | null }> = async () => ({ content: '{}' }) +vi.mock('../detectord/lifecycle', () => ({ + getDetectordClient: () => ({ connect: async () => {}, readConfig: () => readConfigResult() }) +})) import { getHookStatus } from '../runtime/hookStatus' @@ -81,3 +97,43 @@ describe('unmanageable clients', () => { expect(UNKNOWN_AGENT_FACTS.manageable).toBe(true) }) }) + +describe('mcp:readConfig', () => { + let readConfig: (event: unknown, client: string) => Promise<{ content: string | null; error?: string }> + + beforeEach(async () => { + factsCalls = 0 + facts = new Map([['chatgpt' as McpClientId, agent({ manageable: false })]]) + const { registerMcpSubmitHandlers } = await import('../ipc/ipcHandlersMcpSubmit') + registerMcpSubmitHandlers() + readConfig = handlers.get('mcp:readConfig') as typeof readConfig + }) + + it('explains Connectors instead of surfacing an error nobody can act on', async () => { + readConfigResult = async () => { + throw new Error("agent 'chatgpt' has no user-scope config") + } + const { content, error } = await readConfig(null, 'chatgpt') + expect(content).toMatch(/Connectors in your account/) + expect(error).toBeUndefined() + }) + + it('costs no extra discovery pass when the read succeeds', async () => { + // The check used to run first, so every successful read paid for a + // `list_agents` - and AppsStep re-reads every expanded client on refresh, + // turning one wasted scan into one per open panel. + readConfigResult = async () => ({ content: '{"mcpServers":{}}' }) + const { content } = await readConfig(null, 'cursor') + expect(content).toBe('{"mcpServers":{}}') + expect(factsCalls).toBe(0) + }) + + it('still passes a real failure through for a manageable client', async () => { + readConfigResult = async () => { + throw new Error('permission denied') + } + const { content, error } = await readConfig(null, 'cursor') + expect(content).toBeNull() + expect(error).toBe('permission denied') + }) +}) diff --git a/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts b/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts index c7a7959..3732552 100644 --- a/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts +++ b/packages/desktop/src/main/ipc/ipcHandlersMcpSubmit.ts @@ -137,18 +137,6 @@ export function registerMcpSubmitHandlers(): void { // Show an agent's config file. The daemon reads it: it owns agent files, and // this used to take an arbitrary path from the renderer. ipcMain.handle("mcp:readConfig", async (_event, client: string) => { - // An unmanageable client has no local config, so asking the daemon for one - // only produces an error the user can do nothing about. Say what's actually - // going on instead. The daemon is the authority on which those are. - const facts = await getAgentFacts(); - if (facts?.get(client as McpClientId)?.manageable === false) { - const name = CLIENT_DISPLAY[client as McpClientId]?.name ?? client; - return { - content: - `${name} keeps its MCP servers as Connectors in your account, not in a ` + - `local config file. There is nothing here for Edison Watch to read or protect.`, - }; - } try { const daemon = getDetectordClient(); await daemon.connect(); @@ -158,6 +146,24 @@ export function registerMcpSubmitHandlers(): void { // as null content and anything else (permissions, a directory, non-UTF-8) // as an error, so pass the reason on rather than rendering "no config". const message = err instanceof Error ? err.message : String(err); + + // An unmanageable client has no local config, so the failure above is + // expected and its message is one the user can do nothing about. Say + // what's actually going on instead. The daemon is the authority on which + // clients those are, so this asks it - but only here, on a path that has + // already failed. Asking up front cost a `list_agents` (a full discovery + // pass, plus a workspace hook scan) on every successful read, and the + // refresh in AppsStep re-reads every expanded client at once. + const facts = await getAgentFacts(); + if (facts?.get(client as McpClientId)?.manageable === false) { + const name = CLIENT_DISPLAY[client as McpClientId]?.name ?? client; + return { + content: + `${name} keeps its MCP servers as Connectors in your account, not in a ` + + `local config file. There is nothing here for Edison Watch to read or protect.`, + }; + } + console.warn(`[mcp:readConfig] ${client}: ${message}`); return { content: null, error: message }; } From ffa0dc63b2a4fa5ebd225a2df98c970a4d81f176 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 18:47:59 +0000 Subject: [PATCH 5/6] Key unmanageable copy by client, and cover the wire mapping Three of four findings from cubic on #30. The "not protected" wording named ChatGPT's reason while being selected by `manageable`, which is a capability the daemon can set on any client. The next presence-only client would have been told its servers were Connectors in an OpenAI account. Specific advice is now keyed by client id, with a fallback that claims only what the flag itself guarantees. `toFacts` is where `manageable` crosses from daemon JSON into app types, including the `?? true` that keeps an older daemon's omitted field from reading as "unmanageable". Nothing covered it - the existing test asserts on UNKNOWN_AGENT_FACTS, a different constant. Both directions are now tested through the real `getAgentFacts`; flipping the default to `false` fails the second. The ClientsView story replaced `window.api.mcp.getHookStatus` and never put it back. Storybook renders a file's stories on one page, so the next story added here would have inherited these four clients. Swapped in the initialiser and restored on unmount - reading the previous value during render would capture this stub as the "original" on a re-render. --- .../__tests__/unmanageableClients.test.ts | 31 +++++++++- .../components/main/ClientsView.stories.tsx | 62 ++++++++++++------- .../src/components/main/ClientsView.tsx | 33 ++++++++-- 3 files changed, 97 insertions(+), 29 deletions(-) diff --git a/packages/desktop/src/main/__tests__/unmanageableClients.test.ts b/packages/desktop/src/main/__tests__/unmanageableClients.test.ts index 3f52497..8d0a0f6 100644 --- a/packages/desktop/src/main/__tests__/unmanageableClients.test.ts +++ b/packages/desktop/src/main/__tests__/unmanageableClients.test.ts @@ -37,8 +37,13 @@ vi.mock('electron', () => ({ })) let readConfigResult: () => Promise<{ content: string | null }> = async () => ({ content: '{}' }) +let listAgentsResult: Record[] = [] vi.mock('../detectord/lifecycle', () => ({ - getDetectordClient: () => ({ connect: async () => {}, readConfig: () => readConfigResult() }) + getDetectordClient: () => ({ + connect: async () => {}, + readConfig: () => readConfigResult(), + listAgents: async () => listAgentsResult + }) })) import { getHookStatus } from '../runtime/hookStatus' @@ -98,6 +103,30 @@ describe('unmanageable clients', () => { }) }) +/** + * The wire-to-app hop, which the tests above mock past. `manageable` crosses + * here as snake-case JSON from a daemon that may predate the field, and nothing + * upstream of this point is typechecked - the payload is parsed, not compiled. + */ +describe('AgentInfo -> AgentFacts', () => { + // The real module; the rest of this file replaces getAgentFacts with a stub. + const real = async () => await vi.importActual('../detectord/agents') + + it('carries an unmanageable agent through as unmanageable', async () => { + listAgentsResult = [{ name: 'chatgpt', installed: true, manageable: false }] + const facts = await (await real()).getAgentFacts() + expect(facts?.get('chatgpt' as McpClientId)?.manageable).toBe(false) + }) + + it('reads an omitted field as manageable, not as false', async () => { + // The older-daemon case. Defaulting to false here would strip every client + // of its setup conditions the moment the app outran the daemon. + listAgentsResult = [{ name: 'cursor', installed: true }] + const facts = await (await real()).getAgentFacts() + expect(facts?.get('cursor' as McpClientId)?.manageable).toBe(true) + }) +}) + describe('mcp:readConfig', () => { let readConfig: (event: unknown, client: string) => Promise<{ content: string | null; error?: string }> diff --git a/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx b/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx index f1eeffb..761ed66 100644 --- a/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx +++ b/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx @@ -1,3 +1,4 @@ +import { useEffect, useState } from 'react'; import type { Meta, StoryObj } from '@storybook/react-vite'; import ClientsView from './ClientsView'; @@ -35,31 +36,44 @@ const status = (over: Record) => ({ export const WithAnUnmanageableClient: Story = { decorators: [ (Story) => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (window as any).api.mcp.getHookStatus = async () => ({ - statuses: [ - status({ client: 'claude-code' }), - status({ client: 'cursor' }), - status({ - client: 'vscode', - hasHook: false, - hookCount: 2, - mcpConnected: false, - }), - status({ - client: 'chatgpt', - manageable: false, - mcpApplicable: false, - hooksApplicable: false, - hasHook: false, - hookCount: 0, - totalHooks: 0, - mcpConnected: false, - mcpConfigured: false, - }), - ], - daemonUnavailable: false, + // Storybook keeps every story in a file on one page, so a stub assigned + // here outlives the story that set it - the next story added to this file + // would silently inherit these four clients. Swap in the initialiser and + // restore on unmount: reading the previous value on every render would + // capture this stub as the "original" on the second one. + const [restore] = useState(() => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const mcp = (window as any).api.mcp; + const previous = mcp.getHookStatus; + mcp.getHookStatus = async () => ({ + statuses: [ + status({ client: 'claude-code' }), + status({ client: 'cursor' }), + status({ + client: 'vscode', + hasHook: false, + hookCount: 2, + mcpConnected: false, + }), + status({ + client: 'chatgpt', + manageable: false, + mcpApplicable: false, + hooksApplicable: false, + hasHook: false, + hookCount: 0, + totalHooks: 0, + mcpConnected: false, + mcpConfigured: false, + }), + ], + daemonUnavailable: false, + }); + return () => { + mcp.getHookStatus = previous; + }; }); + useEffect(() => restore, [restore]); return (
diff --git a/packages/desktop/src/renderer/src/components/main/ClientsView.tsx b/packages/desktop/src/renderer/src/components/main/ClientsView.tsx index b6798d0..24bce1a 100644 --- a/packages/desktop/src/renderer/src/components/main/ClientsView.tsx +++ b/packages/desktop/src/renderer/src/components/main/ClientsView.tsx @@ -64,6 +64,33 @@ const CLIENT_NAMES: Record = { */ type ClientStatus = "connected" | "partial-setup" | "installed" | "unmanaged" | "missing"; +/** + * Why a given client can't be managed, and what to do about it. + * + * `manageable` is a capability, not an identity: the daemon can mark any client + * unmanageable, and each will be unmanageable for its own reason. Wording that + * assumes ChatGPT's reason would quietly become wrong for the next one, so the + * specific advice is keyed by client and the fallback claims only what the flag + * itself guarantees. + */ +const UNMANAGEABLE_REASON: Record = { + chatgpt: { + row: "Connectors are managed in your account - Edison Watch can't proxy them", + tooltip: + "This app's MCP servers are Connectors held in your account, not local " + + "config Edison Watch can proxy. Remove them and request equivalents from " + + "your admin.", + }, +}; + +const FALLBACK_REASON = { + row: "Edison Watch can't configure this app", + tooltip: "Edison Watch can see this app but cannot configure or protect it.", +}; + +const unmanageableReason = (id: string): { row: string; tooltip: string } => + UNMANAGEABLE_REASON[id] ?? FALLBACK_REASON; + function StatusDot({ status }: { status: ClientStatus }) { const colors: Record = { connected: "bg-emerald-400", @@ -137,9 +164,7 @@ function ConditionTooltip({ client }: { client: ClientInfo }) { Not protected

- This app's MCP servers are Connectors held in your account, not - local config Edison Watch can proxy. Remove them and request - equivalents from your admin. + {unmanageableReason(client.id).tooltip}

@@ -393,7 +418,7 @@ export default function ClientsView(): React.ReactNode { status === "partial-setup" ? getIssueDetail(client) : status === "unmanaged" - ? "Connectors are managed in your account - Edison Watch can't proxy them" + ? unmanageableReason(client.id).row : null; return ( From b9dcc97e53e2b6aca5e1f19491b6f4961d8f1f2c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 18:59:37 +0000 Subject: [PATCH 6/6] Make the story mock symmetric, and look up client ids safely Both findings from cubic's second pass, on the fixes from ffa0dc6. The story installed its stub during render and removed it in an effect. React repeats renders - StrictMode double-invokes, concurrent renders get thrown away - and a repeat would capture the stub itself as the value to restore. Install and restore now both live in the effect, so they cannot disagree. `useLayoutEffect` because ClientsView fetches in a passive effect and every layout effect runs before any passive one. Client ids come from the daemon and were read out of object literals, which answer inherited keys as though they were entries: a client named `constructor` took a function where the fallback belongs. Both maps in this file are now Maps, which have no such keys. `CLIENT_NAMES` was not flagged but has the same lookup and would have rendered that function as a display name. Covers ClientsView for the first time: a known unmanageable client gets its specific advice, an unknown one gets the generic fallback. The unknown id in the test is `constructor`, so the test fails against the object-literal lookup. --- .../src/__tests__/renderSmoke.test.tsx | 40 +++++++++++ .../components/main/ClientsView.stories.tsx | 20 ++++-- .../src/components/main/ClientsView.tsx | 69 +++++++++++-------- 3 files changed, 94 insertions(+), 35 deletions(-) diff --git a/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx b/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx index d7aadef..020351a 100644 --- a/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx +++ b/packages/desktop/src/renderer/src/__tests__/renderSmoke.test.tsx @@ -16,6 +16,7 @@ vi.mock('../components/onboarding/PersonalKeyCard', () => ({ })) import AppsStep from '../components/onboarding/AppsStep' import MainMenu from '../components/main/MainMenu' +import ClientsView from '../components/main/ClientsView' import EncryptionStep from '../components/onboarding/EncryptionStep' /** @@ -151,6 +152,45 @@ describe('MainMenu', () => { }) }) +describe('ClientsView', () => { + const status = (over: Record) => ({ + installed: true, + hasHook: true, + hookCount: 4, + totalHooks: 4, + mcpConnected: true, + mcpConfigured: true, + mcpApplicable: true, + hooksApplicable: true, + manageable: true, + ...over + }) + + it('explains an unmanageable client it knows, and one it does not', async () => { + // `manageable` is a capability the daemon can set on any client, so the + // copy cannot assume ChatGPT's reason. An unrecognised id has to fall back + // to what the flag alone guarantees rather than borrowing that wording. + // + // `constructor` is the unrecognised id on purpose: the ids are chosen by + // the daemon, and an object lookup answers inherited keys as if they were + // entries, so it would take a function where the fallback belongs. + const api = installMockApi() + ;(api.mcp as Record).getHookStatus = async () => ({ + statuses: [ + status({ client: 'chatgpt', manageable: false, mcpApplicable: false, hooksApplicable: false }), + status({ client: 'constructor', manageable: false, mcpApplicable: false, hooksApplicable: false }) + ], + daemonUnavailable: false + }) + + render() + + expect(await screen.findByText(/Connectors are managed in your account/)).toBeTruthy() + expect(screen.getByText(/^Edison Watch can't configure this app$/)).toBeTruthy() + expectNoRenderErrors() + }) +}) + describe('EncryptionStep', () => { const props = { mcpBaseUrl: 'https://mcp.example', diff --git a/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx b/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx index 761ed66..fbafdca 100644 --- a/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx +++ b/packages/desktop/src/renderer/src/components/main/ClientsView.stories.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react'; +import { useLayoutEffect } from 'react'; import type { Meta, StoryObj } from '@storybook/react-vite'; import ClientsView from './ClientsView'; @@ -38,10 +38,17 @@ export const WithAnUnmanageableClient: Story = { (Story) => { // Storybook keeps every story in a file on one page, so a stub assigned // here outlives the story that set it - the next story added to this file - // would silently inherit these four clients. Swap in the initialiser and - // restore on unmount: reading the previous value on every render would - // capture this stub as the "original" on the second one. - const [restore] = useState(() => { + // would silently inherit these four clients. + // + // Swap and restore both happen in the effect, so they stay symmetric: + // mutating a global during render is not safe to repeat, and React does + // repeat renders (StrictMode double-invokes, concurrent renders can be + // thrown away). A second pass would capture this stub as the "previous" + // value and restore it on unmount. `useLayoutEffect` rather than + // `useEffect` because ClientsView fetches in a passive effect, and every + // layout effect runs before any passive one - so the stub is in place + // before the component asks. + useLayoutEffect(() => { // eslint-disable-next-line @typescript-eslint/no-explicit-any const mcp = (window as any).api.mcp; const previous = mcp.getHookStatus; @@ -72,8 +79,7 @@ export const WithAnUnmanageableClient: Story = { return () => { mcp.getHookStatus = previous; }; - }); - useEffect(() => restore, [restore]); + }, []); return (
diff --git a/packages/desktop/src/renderer/src/components/main/ClientsView.tsx b/packages/desktop/src/renderer/src/components/main/ClientsView.tsx index 24bce1a..4d0cefb 100644 --- a/packages/desktop/src/renderer/src/components/main/ClientsView.tsx +++ b/packages/desktop/src/renderer/src/components/main/ClientsView.tsx @@ -38,21 +38,23 @@ interface ClientInfo { } // Map client IDs (from McpClientId) to display names. Every id the daemon can -// report needs an entry - a missing one renders as the raw id. -const CLIENT_NAMES: Record = { - "claude-code": "Claude Code", - "claude-desktop": "Claude Desktop", - "claude-cowork": "Claude Cowork", - chatgpt: "ChatGPT", - cursor: "Cursor", - windsurf: "Windsurf (early alpha)", - codex: "Codex", - vscode: "VS Code", - zed: "Zed (early alpha)", - intellij: "IntelliJ IDEA (early alpha)", - pycharm: "PyCharm", - webstorm: "WebStorm", -}; +// report needs an entry - a missing one renders as the raw id. A Map for the +// same reason as UNMANAGEABLE_REASON below: the key comes from the daemon, and +// an object lookup would answer `constructor` with a function to render. +const CLIENT_NAMES = new Map([ + ["claude-code", "Claude Code"], + ["claude-desktop", "Claude Desktop"], + ["claude-cowork", "Claude Cowork"], + ["chatgpt", "ChatGPT"], + ["cursor", "Cursor"], + ["windsurf", "Windsurf (early alpha)"], + ["codex", "Codex"], + ["vscode", "VS Code"], + ["zed", "Zed (early alpha)"], + ["intellij", "IntelliJ IDEA (early alpha)"], + ["pycharm", "PyCharm"], + ["webstorm", "WebStorm"], +]); /** * `unmanaged` is installed-and-outside-our-reach: a client whose MCP servers @@ -73,23 +75,34 @@ type ClientStatus = "connected" | "partial-setup" | "installed" | "unmanaged" | * specific advice is keyed by client and the fallback claims only what the flag * itself guarantees. */ -const UNMANAGEABLE_REASON: Record = { - chatgpt: { - row: "Connectors are managed in your account - Edison Watch can't proxy them", - tooltip: - "This app's MCP servers are Connectors held in your account, not local " + - "config Edison Watch can proxy. Remove them and request equivalents from " + - "your admin.", - }, -}; +interface UnmanageableReason { + row: string; + tooltip: string; +} + +// A Map, not an object literal: the key is an id the daemon chose, and an +// object lookup answers inherited keys as though they were entries - a client +// named `constructor` would take a function where the fallback belongs. +const UNMANAGEABLE_REASON = new Map([ + [ + "chatgpt", + { + row: "Connectors are managed in your account - Edison Watch can't proxy them", + tooltip: + "This app's MCP servers are Connectors held in your account, not local " + + "config Edison Watch can proxy. Remove them and request equivalents from " + + "your admin.", + }, + ], +]); -const FALLBACK_REASON = { +const FALLBACK_REASON: UnmanageableReason = { row: "Edison Watch can't configure this app", tooltip: "Edison Watch can see this app but cannot configure or protect it.", }; -const unmanageableReason = (id: string): { row: string; tooltip: string } => - UNMANAGEABLE_REASON[id] ?? FALLBACK_REASON; +const unmanageableReason = (id: string): UnmanageableReason => + UNMANAGEABLE_REASON.get(id) ?? FALLBACK_REASON; function StatusDot({ status }: { status: ClientStatus }) { const colors: Record = { @@ -229,7 +242,7 @@ export default function ClientsView(): React.ReactNode { setClients( statuses.map((s) => ({ id: s.client, - name: CLIENT_NAMES[s.client] ?? s.client, + name: CLIENT_NAMES.get(s.client) ?? s.client, installed: s.installed, hasHook: s.hasHook, hookCount: s.hookCount ?? 0,