Detect the ChatGPT desktop app as a partially-supported client - #30
Detect the ChatGPT desktop app as a partially-supported client#30Miyamura80 wants to merge 7 commits into
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqPs27HH3TkQ6hQeJ5mAK
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<dyn Agent>` 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGqPs27HH3TkQ6hQeJ5mAK
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqPs27HH3TkQ6hQeJ5mAK
PR Summary by QodoDetect ChatGPT desktop app as installed-but-unmanageable client
AI Description
Diagram
High-Level Assessment
Files changed (26)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
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.
|
All three review findings fixed in e58a16e.
Fixed by moving the check after the read rather than by matching on the daemon's error string, which would have coupled this handler to error text the daemon is free to reword. An unmanageable client always fails the read, so the check still runs whenever it can change the answer, and never when it can't. Same output, one RPC on the happy path.
macOS test assuming a home dir — correct, and the sharper version of the point is that the test asserted more than the code promises, so it could fail on a state the code handles correctly. Now asserts 2 candidates per bundle when The handler had no test coverage at all before this, which is how the ordering problem got in. Added three: the Connectors message, passthrough of a genuine error, and a guard asserting no discovery pass on a successful read. The guard was mutation-checked — it fails against the previous ordering. Green locally: 195 desktop tests, 156 Rust tests, clippy and fmt clean. Generated by Claude Code |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/desktop/src/main/discovery/types.ts">
<violation number="1" location="packages/desktop/src/main/discovery/types.ts:20">
P2: ChatGPT can now be represented as the client of a discovered MCP server, which conflicts with the stated detect-only contract and can route such an entry into submission. A separate installed/detectable-client type (or a server-client type that excludes `chatgpt`) would keep ChatGPT presence-only at the type boundary.</violation>
</file>
<file name="packages/desktop/src/main/detectord/agents.ts">
<violation number="1" location="packages/desktop/src/main/detectord/agents.ts:61">
P3: The daemon-to-UI `manageable` boundary is not covered by the current tests, so a regression could silently turn ChatGPT into a selectable client or lose the older-daemon fallback. A focused `getAgentFacts`/protocol-mapping test covering both `manageable: false` and an omitted field would protect this compatibility behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // 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' |
There was a problem hiding this comment.
P2: ChatGPT can now be represented as the client of a discovered MCP server, which conflicts with the stated detect-only contract and can route such an entry into submission. A separate installed/detectable-client type (or a server-client type that excludes chatgpt) would keep ChatGPT presence-only at the type boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop/src/main/discovery/types.ts, line 20:
<comment>ChatGPT can now be represented as the client of a discovered MCP server, which conflicts with the stated detect-only contract and can route such an entry into submission. A separate installed/detectable-client type (or a server-client type that excludes `chatgpt`) would keep ChatGPT presence-only at the type boundary.</comment>
<file context>
@@ -14,6 +14,10 @@ export type McpClientId =
+ // 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'
</file context>
There was a problem hiding this comment.
Not fixing this one — leaving the thread open so a human can overrule.
The type is wider than reality, agreed. But the reachability claim doesn't hold: ChatGpt::discover() returns Ok(Vec::new()) unconditionally, and the daemon is the only source of discovered servers, so no entry can carry client: 'chatgpt' to route into submission.
Splitting McpClientId into installed-client and server-client variants would touch every discovery and submission signature to encode a state the daemon cannot produce. And since daemon payloads are parsed rather than compiled, the boundary would still need a cast — which is exactly where the guarantee would be lost, so the refactor would buy less safety than it appears to.
Worth revisiting if ChatGPT ever gains a local config, since that's the change that would make this reachable.
Generated by Claude Code
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.
|
Three of cubic's four fixed in ffa0dc6. Declining the fourth, with reasoning.
Story leaks its
Green locally: 197 desktop tests, both typecheck projects clean. Generated by Claude Code |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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.
|
Both fixed in b9dcc97. Fair catches — these were on my previous round of fixes, not the original PR. Story mock installed during render (P2) — correct. I'd moved the swap into a Used Object lookup answering inherited keys (P3) — correct. Fixed at the data structure rather than the call site: both maps are now Extended it to
Green locally: 198 desktop tests, both typecheck projects clean. Generated by Claude Code |
2 participants
Reimplements edison-watch#1064, whose changes were lost when the desktop client moved into this repo. The original targeted the Electron main process; detection now lives in the Rust daemon, so this is a reimplementation rather than a cherry-pick.
What it does
ChatGPT keeps its MCP servers as Connectors in the user's OpenAI account, not in a local config file. Edison can see the app is installed but can never configure it. Previously it was simply absent from onboarding, so a user with ChatGPT installed finished setup believing every MCP host on their machine was protected.
ChatGPT now appears in both the onboarding wizard and the permanent clients view, marked "Not protected" and excluded from selection, hook counts, and config writes.
Design
Agent::is_manageable()is a declared fact on the Rust trait, not inferred from an emptyedison_installs(). JetBrains with no IDE installed returns empty yet is perfectly manageable, so inference would misclassify it.That fact flows outward as data rather than being re-derived per call site:
The daemon is the enforcement point.
retain_manageable()runs inside bothenrollandapply_integrations, because those are two independent entry paths and enrollment is additive: an app-side filter guarding only one of them would letchatgptintoselected_agentspermanently. Unknown agent names are preserved, so an older app talking to a newer daemon does not lose enrollments.AgentInfo.manageablecarries#[serde(default = "default_true")], so an older daemon that omits the field does not break a newer app.Also fixed
heal_edison_installcounted and logged a heal outside theedison_installs()loop, so an agent with nothing to install still reported as healed. The self-heal signal was permanently non-zero. It now counts only what it actually wrote.Verification
clippy --all-targets --all-features,fmt --checkall cleanNew behavioural tests were mutation-checked against the unfixed code to confirm they fail without the fix.
Known gaps
default_app_paths()returns empty there and detection never fires. Still true after the July 2026 Codex/ChatGPT merger, which shipped macOS and Windows only.%LOCALAPPDATA%\Microsoft\WindowsApps\ChatGPT.exefollows the MSIX execution-alias convention but has not been checked against a real install. If the filename is wrong, detection silently never fires on Windows. Worth a spot-check by someone with a Windows box.Note for reviewers
Building the detectord workspace needs a newer rustc than the crate's declared
rust-version = "1.88", becauselibsqlite3-sys0.38.1 usescfg_select. CI resolvesstableso it is unaffected, but a pinned older local toolchain will fail to build.Generated by Claude Code
Summary by cubic
Detects the ChatGPT desktop app as a presence-only client and shows it as “Not protected.” Moves “manageable” into the daemon, removes extra discovery on config reads, adds client‑specific guidance for unmanageable apps, and hardens client id lookups and story mocks.
New Features
chatgptagent inedison-detectord: presence via app path (macOSChatGPT.app/ChatGPT Classic.app; Windows%LOCALAPPDATA%\Microsoft\WindowsApps\ChatGPT.exeandPrograms\ChatGPT\ChatGPT.exe; none on Linux). Discovers no servers and is never an install target.Agent::is_manageable()and plumbed toAgentInfo.manageable(defaults to true). IPC/preload carry it;McpClientIdincludeschatgpt.mcp:readConfig: on failure for an unmanageable client, returns an explanatory message; the success path no longer triggers alist_agentsscan.Bug Fixes
enrollandapply_integrations, and prunes any stale names; preventschatgptfrom persisting in selections.LazyLock) to avoid rebuilding agents and re‑emitting discovery warnings during selection filtering.heal_edison_installonly counts/logs when it writes an entry, fixing a permanently non‑zero self‑heal signal on agents with no install targets.manageable(older daemons) as true; tests cover the wire mapping. Storybook now restores API stubs after use.Written for commit b9dcc97. Summary will update on new commits.