fix(mcp): tell the user when the MCP server or plugin is already installed - #1075
Merged
Merged
Conversation
…alled `mcp add` collapsed "already installed" into an empty result set, so a re-run printed "Installation skipped." with no reason and, because the outcome was Failed, skipped the follow-on steps and exited immediately. Codex hit this every time: `codex plugin marketplace add` exits non-zero once the marketplace is registered. Install results are now per-client (installed / already-installed / failed) with a failure reason, and the Done screen renders each group with copy that says what happened. Already-installed counts as a working install, so the flow continues instead of quitting. Also in here, found while tracing the same flow: - Codex asks config.toml (and Claude Code asks `plugin list`) before installing, instead of inferring the no-op from CLI error text. - CLI error text is redacted before it reaches the log, the screen or an exception report — the failing command echoes back the Authorization header. - The tri-state install mode is passed to the installer rather than read from state set in the same tick, which made a single-editor machine take the MCP-only path after choosing "Install with all features". Generated-By: PostHog Code Task-Id: ae3d3411-9e66-468b-aa6c-35ee0a410249
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Results will be posted here when complete. |
…aiming success Auditing the rest of the flow for the same class of bug turned up four more places where a failure produced no copy, or the wrong copy: - `mcp remove` reported every client detected before the attempt as removed, because removeMCPServer discarded each removeServer result. Removals now return per-client results, so a failed removal says so with a reason. - The non-interactive `mcp remove` printed nothing at all — no success, no failure, not even "nothing to remove". - A crash in client detection rendered as "No supported MCP clients detected", identical to genuinely having none; a crash during install/remove rendered as "no editor was selected". Both now name the error and point at --debug. - `mcp remove --local` never targeted posthog-local: the flag was dropped by McpInstaller.remove() and ignored by Codex's removeServer. Also: `mcp remove` fell back to LoggingUI on any TUI error, masking real bugs as a missing TTY — it now uses the same isTUIUnavailable guard as `mcp add`, moved to a shared module. Codex failure reasons are redacted like Claude Code's. McpClientStatus members are renamed Changed/Unchanged since both flows report through them now. Generated-By: PostHog Code Task-Id: ae3d3411-9e66-468b-aa6c-35ee0a410249
edwinyjlim
marked this pull request as ready for review
August 10, 2026 19:40
Contributor
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 3 consider. Published 4 findings (view the review). |
gewenyu99
approved these changes
Aug 11, 2026
gewenyu99
left a comment
gewenyu99
left a comment
Collaborator
There was a problem hiding this comment.
Screen.Recording.2026-08-11.at.1.15.23.PM.mov
Screen.Recording.2026-08-11.at.1.16.11.PM.mov
2 requests:
- Make this require a click to dismiss or give me like 20 seconds to read it. It flashes by with too much text
- More padding between items
Otherwise, this works great!
Sending Code review to review the actual code, it looks good on vibes but I'm not super familiar with the MCP code touched here
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ReviewHog Report
Bugfix
Issues: 3 issues
Files (5)
src/steps/add-mcp-server-to-clients/MCPClient.tssrc/steps/add-mcp-server-to-clients/clients/claude-code.tssrc/steps/add-mcp-server-to-clients/clients/claude-web.tssrc/steps/add-mcp-server-to-clients/clients/codex.tssrc/steps/add-mcp-server-to-clients/plugin-client.ts
What were the main changes
- File-based MCPClient now compares the existing config entry and skips the write when identical, returning alreadyInstalled instead of silently rewriting or failing
- MCPClient.removeServer reports alreadyInstalled (not a bare failure) when there is no config file or no PostHog entry left to delete
- Codex checks config.toml via isPluginInstalled before running
plugin marketplace add, avoiding reliance on CLI stderr text for the no-op case; distinguishes real 'already installed' wording from the stale-marketplace-cache case - Codex removeServer now honors the
localflag (previously always targeted 'posthog', ignoring 'posthog-local') - Claude Code checks
plugin listbefore installing and redacts secrets from captured error/reason text; addServer/removeServer/installPlugin return structured InstallResult with reasons - claude-web removeServer now returns an explanatory reason pointing at claude.ai settings instead of a bare failure
- PluginInstallResult now aliases the shared InstallResult type instead of its own interface
Frontend
Issues: 1 issue
Files (6)
src/ui/tui/screens/McpScreen.tsxsrc/ui/tui/services/mcp-installer.tssrc/ui/tui/playground/demos/McpDemo.tsxsrc/commands/mcp/add.tssrc/commands/mcp/remove.tssrc/commands/mcp/tui-availability.ts
What were the main changes
- McpScreen Done phase now renders separate result groups (installed / already-installed / failed, with reasons) instead of a bare 'Installation skipped.' message
- Detection and install/remove crashes now surface the actual error text and point at --debug, instead of reading as 'no clients detected' or 'no editor selected'
- Already-installed now counts as a successful outcome so the flow continues to Slack/prompt steps instead of stopping early
- Fixed installMode closure bug: doInstall now takes the chosen mode explicitly instead of reading stale state, which previously sent single-editor machines down the MCP-only path after choosing 'install with all features'
- mcp-installer service now returns McpClientResult[] from install/remove/installPlugins, forwards the
localflag on remove (previously dropped), and redacts secrets before logging - Extracted isTUIUnavailable into a shared tui-availability.ts module; mcp remove now uses the same guard as mcp add instead of falling back to LoggingUI on any TUI error
- McpDemo playground updated to produce mixed installed/already-installed/failed results for manual testing
Comment on lines
232
to
+239
| const msg = error instanceof Error ? error.message : String(error); | ||
| if (msg.includes('already installed') || msg.includes('already exists')) { | ||
| return Promise.resolve({ success: true, alreadyInstalled: true }); | ||
| return { success: true, alreadyInstalled: true }; | ||
| } | ||
| analytics.captureException( | ||
| new Error(`Claude Code plugin install failed: ${msg}`), | ||
| ); | ||
| return Promise.resolve({ success: false }); | ||
| return { success: false, reason: msg }; |
Contributor
There was a problem hiding this comment.
Claude plugin failures bypass secret redaction
Why we think it's a valid issue
- Checked:
installPlugincatch block vs. its siblings at PR headcbf31a7, plus how the returnedreasonis consumed (installPlugins→toClientResult→summarizeFailure) and whatredactSecretsmasks. - Found: The inconsistency is real —
installPluginuses rawmsgat claude-code.ts:232 for bothcaptureException(236-238) andreturn { success:false, reason: msg }(239), whileaddServer(150-161) andremoveServer(180-191) both wrap error text inredactSecrets. - Found: The reviewer's UI claim is factually wrong. The returned
reasonis redacted downstream:toClientResultsetsdetail: summarizeFailure(result?.reason)(results.ts:66) andsummarizeFailurecallsredactSecrets(results.ts:49-50) before the text ever reaches a screen. The only genuinely-unredacted sink isanalytics.captureException. - Found: No concrete secret-leak trigger exists on this path.
installPlugin()takes no args and runsclaude plugin install posthog— no apiKey, noAuthorization: Bearer, nophx_/phc_key on the command line. This is unlikeaddServer, whose command embeds--header "Authorization: Bearer <apiKey>"(a real, nameable leak the redaction there fixes). For a secret to enter this error text the Claude CLI would have to spontaneously echo stored credentials on a plugin-install failure — speculative, not a realistic failure mode. - Impact: Because
removeServer's command (claude mcp remove --scope user posthog) also carries no command-line secret yet is still redacted, the author clearly intends to redact all claude-CLI error output defensively;installPluginis a genuine gap in that pattern and the one-line fix is trivial and zero-risk. But the practical security impact is negligible — the UI sink is already masked and no secret reaches the exception sink — so it does not meet the bar for a reachable secret leak. - Priority:
must_fixis overstated (no reachable secret exposure, and the UI-exposure half of the claim is incorrect). Down-ranked toconsider: a legitimate defense-in-depth consistency fix that matches the author's own redaction pattern, but not a merge-blocking security bug.
Issue description
The plugin-install failure path uses the raw child-process error for analytics and the returned reason. Unlike the MCP add/remove paths, this can expose sensitive CLI output to exception reporting and the UI.
Suggested fix
Pass the error through redactSecrets before checking it, capturing it, or returning it: const msg = redactSecrets(error instanceof Error ? error.message : String(error));.
Prompt to fix with AI (copy-paste)
## Context
@src/steps/add-mcp-server-to-clients/clients/claude-code.ts#L232-239
<issue_description>
The plugin-install failure path uses the raw child-process error for analytics and the returned `reason`. Unlike the MCP add/remove paths, this can expose sensitive CLI output to exception reporting and the UI.
</issue_description>
<issue_validation>
- **Checked:** `installPlugin` catch block vs. its siblings at PR head `cbf31a7`, plus how the returned `reason` is consumed (`installPlugins` → `toClientResult` → `summarizeFailure`) and what `redactSecrets` masks.
- **Found:** The inconsistency is real — `installPlugin` uses raw `msg` at claude-code.ts:232 for both `captureException` (236-238) and `return { success:false, reason: msg }` (239), while `addServer` (150-161) and `removeServer` (180-191) both wrap error text in `redactSecrets`.
- **Found:** The reviewer's *UI* claim is factually wrong. The returned `reason` is redacted downstream: `toClientResult` sets `detail: summarizeFailure(result?.reason)` (results.ts:66) and `summarizeFailure` calls `redactSecrets` (results.ts:49-50) before the text ever reaches a screen. The only genuinely-unredacted sink is `analytics.captureException`.
- **Found:** No concrete secret-leak trigger exists on this path. `installPlugin()` takes no args and runs `claude plugin install posthog` — no apiKey, no `Authorization: Bearer`, no `phx_`/`phc_` key on the command line. This is unlike `addServer`, whose command embeds `--header "Authorization: Bearer <apiKey>"` (a real, nameable leak the redaction there fixes). For a secret to enter this error text the Claude CLI would have to spontaneously echo stored credentials on a plugin-install failure — speculative, not a realistic failure mode.
- **Impact:** Because `removeServer`'s command (`claude mcp remove --scope user posthog`) also carries no command-line secret yet is still redacted, the author clearly intends to redact *all* claude-CLI error output defensively; `installPlugin` is a genuine gap in that pattern and the one-line fix is trivial and zero-risk. But the practical security impact is negligible — the UI sink is already masked and no secret reaches the exception sink — so it does not meet the bar for a reachable secret leak.
- **Priority:** `must_fix` is overstated (no reachable secret exposure, and the UI-exposure half of the claim is incorrect). Down-ranked to `consider`: a legitimate defense-in-depth consistency fix that matches the author's own redaction pattern, but not a merge-blocking security bug.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Pass the error through `redactSecrets` before checking it, capturing it, or returning it: `const msg = redactSecrets(error instanceof Error ? error.message : String(error));`.
</potential_solution>
Comment on lines
+166
to
+168
| // `mcp add` looked like a failure and reported nothing at all. | ||
| if (await this.isPluginInstalled()) { | ||
| return { success: true, alreadyInstalled: true }; |
Contributor
There was a problem hiding this comment.
Commented TOML can falsely mark the Codex plugin as installed
Why we think it's a valid issue
- Checked:
isPluginInstalled(codex.ts:143-154), the pre-install gate that consumes it (codex.ts:167-168), and the install's downstream self-correction path (codex.ts:198-210). - Found: The check is a raw substring match —
contents.toLowerCase().includes('[marketplaces.posthog]')(codex.ts:149) — so a commented line like# [marketplaces.posthog]matches, and the gate returns{ success: true, alreadyInstalled: true }at codex.ts:167-168 without running the install. - Found: The false-positive direction is uncorrected. The
ALREADY_INSTALLED_PATTERNstderr check (codex.ts:205-210) only self-corrects the reverse miss (pre-check false-negative → install still runs → stderr caught). A false positive skips theplugin marketplace addentirely, so nothing downstream fixes it. - Impact: When
[marketplaces.posthog]exists only in commented/inactive form, the wizard reports 'Plugin already installed for: Codex' while Codex has no active marketplace entry — a silently broken install reported as success. This is a genuine functional defect (unlike a benign rewrite), which is why it clears the bar. - Impact (why not higher): Both normal states behave correctly — never-installed configs lack the string, actively-installed configs contain the live table. The false positive needs a commented-only header (hand-disabled entry or a
#-prefixed example), an uncommon state; and substring detection is the accepted pattern across these CLI clients (e.g. claude-code'smcp list.includes(serverName)), so this isn't uniquely fragile. - Priority: Downgraded to
consider— the harmful outcome is real but the commented-only-header trigger is low-frequency, soshould_fixoverstates how often it bites; the reviewer's minimal fix (ignore comment lines / anchor the header) is proportionate and worth keeping on record.
Issue description
The new pre-install check relies on isPluginInstalled(), which performs a raw substring search for [marketplaces.posthog]. A commented-out example or stale commented configuration containing that header therefore makes installPlugin skip installation and report alreadyInstalled even though Codex has no active marketplace entry.
Suggested fix
Parse config.toml with a TOML parser and check the active marketplaces.posthog table. At minimum, inspect only non-comment lines and require an exact table-header match.
Prompt to fix with AI (copy-paste)
## Context
@src/steps/add-mcp-server-to-clients/clients/codex.ts#L166-168
<issue_description>
The new pre-install check relies on isPluginInstalled(), which performs a raw substring search for `[marketplaces.posthog]`. A commented-out example or stale commented configuration containing that header therefore makes installPlugin skip installation and report alreadyInstalled even though Codex has no active marketplace entry.
</issue_description>
<issue_validation>
- **Checked:** `isPluginInstalled` (codex.ts:143-154), the pre-install gate that consumes it (codex.ts:167-168), and the install's downstream self-correction path (codex.ts:198-210).
- **Found:** The check is a raw substring match — `contents.toLowerCase().includes('[marketplaces.posthog]')` (codex.ts:149) — so a commented line like `# [marketplaces.posthog]` matches, and the gate returns `{ success: true, alreadyInstalled: true }` at codex.ts:167-168 without running the install.
- **Found:** The false-positive direction is uncorrected. The `ALREADY_INSTALLED_PATTERN` stderr check (codex.ts:205-210) only self-corrects the *reverse* miss (pre-check false-negative → install still runs → stderr caught). A false positive skips the `plugin marketplace add` entirely, so nothing downstream fixes it.
- **Impact:** When `[marketplaces.posthog]` exists only in commented/inactive form, the wizard reports 'Plugin already installed for: Codex' while Codex has no active marketplace entry — a silently broken install reported as success. This is a genuine functional defect (unlike a benign rewrite), which is why it clears the bar.
- **Impact (why not higher):** Both normal states behave correctly — never-installed configs lack the string, actively-installed configs contain the live table. The false positive needs a commented-only header (hand-disabled entry or a `#`-prefixed example), an uncommon state; and substring detection is the accepted pattern across these CLI clients (e.g. claude-code's `mcp list` `.includes(serverName)`), so this isn't uniquely fragile.
- **Priority:** Downgraded to `consider` — the harmful outcome is real but the commented-only-header trigger is low-frequency, so `should_fix` overstates how often it bites; the reviewer's minimal fix (ignore comment lines / anchor the header) is proportionate and worth keeping on record.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Parse config.toml with a TOML parser and check the active `marketplaces.posthog` table. At minimum, inspect only non-comment lines and require an exact table-header match.
</potential_solution>
Comment on lines
+104
to
+106
| const reason = redactSecrets(stderr); | ||
| analytics.captureException(new Error(`Codex MCP add failed: ${reason}`)); | ||
| return Promise.resolve({ success: false, reason }); |
Contributor
There was a problem hiding this comment.
Codex spawn failures produce empty failure reasons
Why we think it's a valid issue
- Checked: The
addServer(codex.ts:99-106) andinstallPlugin(codex.ts:198-215) failure branches vs. the siblingremoveServer(codex.ts:126-133), plusspawnSynclaunch-failure semantics andfindCodexBinary(codex.ts:41-55). - Found: Both flagged paths gate on
if (result.status !== 0)and derive the reason solely from stderr:const stderr = result.stderr ?? ''→redactSecrets(stderr). On a launch failurespawnSyncreturns{ error, status: null, stderr: null }, so the guard still fires (null !== 0), butstderr ?? ''is'',ALREADY_INSTALLED_PATTERN.test('')is false, and the reason is empty — the real diagnostic inresult.erroris dropped from both the returnedreasonandanalytics.captureException(new Error('Codex ... failed: ')). - Found:
removeServeralready does it right at codex.ts:126-128 —result.error || result.status !== 0andredactSecrets(result.error?.message ?? result.stderr ?? 'codex mcp remove failed')— so this is a gap in the author's own pattern across the same file, not reviewer paranoia. - Impact: On spawn launch failures (EACCES on a non-executable resolved path, ENOENT on a dangling symlink, or the cached
findCodexBinarypath going stale between detection and install — a real TOCTOU window since the path is resolved once and reused), the user sees 'Couldn't install for: Codex — ' with no reason and telemetry gets an empty diagnostic. That is a swallowed error that hides a failure and directly undermines the PR's central goal of meaningful failure reasons. - Impact (scope): It only bites launch failures, not normal non-zero exits (where stderr is populated), so it is a subset of failures — but that is precisely the case where the diagnostic matters most and is lost.
- Priority: Left at
should_fix— real reliability/observability defect, trivial proportionate fix (mirrorremoveServer), the author already established the pattern in-file; not a crash/data-loss so not must_fix.
Issue description
The add and plugin-install failure paths derive the reason only from stderr. When spawnSync fails to launch the cached binary, status is null and the diagnostic is in result.error, leaving users and exception telemetry with an empty reason. The remove path already handles this correctly.
Suggested fix
Build the reason consistently as result.error?.message ?? result.stderr ?? '<operation> failed', then redact it before returning or reporting it. Consider a shared helper for all Codex subprocess results.
Prompt to fix with AI (copy-paste)
## Context
@src/steps/add-mcp-server-to-clients/clients/codex.ts#L104-106
@src/steps/add-mcp-server-to-clients/clients/codex.ts#L199-215
<issue_description>
The add and plugin-install failure paths derive the reason only from `stderr`. When `spawnSync` fails to launch the cached binary, `status` is null and the diagnostic is in `result.error`, leaving users and exception telemetry with an empty reason. The remove path already handles this correctly.
</issue_description>
<issue_validation>
- **Checked:** The `addServer` (codex.ts:99-106) and `installPlugin` (codex.ts:198-215) failure branches vs. the sibling `removeServer` (codex.ts:126-133), plus `spawnSync` launch-failure semantics and `findCodexBinary` (codex.ts:41-55).
- **Found:** Both flagged paths gate on `if (result.status !== 0)` and derive the reason solely from stderr: `const stderr = result.stderr ?? ''` → `redactSecrets(stderr)`. On a launch failure `spawnSync` returns `{ error, status: null, stderr: null }`, so the guard still fires (`null !== 0`), but `stderr ?? ''` is `''`, `ALREADY_INSTALLED_PATTERN.test('')` is false, and the reason is empty — the real diagnostic in `result.error` is dropped from both the returned `reason` and `analytics.captureException(new Error('Codex ... failed: '))`.
- **Found:** `removeServer` already does it right at codex.ts:126-128 — `result.error || result.status !== 0` and `redactSecrets(result.error?.message ?? result.stderr ?? 'codex mcp remove failed')` — so this is a gap in the author's own pattern across the same file, not reviewer paranoia.
- **Impact:** On spawn launch failures (EACCES on a non-executable resolved path, ENOENT on a dangling symlink, or the cached `findCodexBinary` path going stale between detection and install — a real TOCTOU window since the path is resolved once and reused), the user sees 'Couldn't install for: Codex — ' with no reason and telemetry gets an empty diagnostic. That is a swallowed error that hides a failure and directly undermines the PR's central goal of meaningful failure reasons.
- **Impact (scope):** It only bites launch failures, not normal non-zero exits (where stderr is populated), so it is a subset of failures — but that is precisely the case where the diagnostic matters most and is lost.
- **Priority:** Left at `should_fix` — real reliability/observability defect, trivial proportionate fix (mirror `removeServer`), the author already established the pattern in-file; not a crash/data-loss so not must_fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Build the reason consistently as `result.error?.message ?? result.stderr ?? '<operation> failed'`, then redact it before returning or reporting it. Consider a shared helper for all Codex subprocess results.
</potential_solution>
Comment on lines
+155
to
+164
| const already = namesWithStatus(results, McpClientStatus.Unchanged); | ||
| analytics.wizardCapture('mcp plugins installed', { | ||
| clients: installed, | ||
| // `clients` keeps its original meaning — every client that ended up with | ||
| // the plugin — so existing insights don't dip when a re-run reports | ||
| // already-installed instead of a fresh write. | ||
| clients: [ | ||
| ...namesWithStatus(results, McpClientStatus.Changed), | ||
| ...already, | ||
| ], | ||
| already_installed: already, |
Contributor
There was a problem hiding this comment.
Plugin failure analytics omit the new failed breakdown
Why we think it's a valid issue
- Checked: All three MCP analytics events added/modified in this PR, plus whether plugin installs can actually yield a
Failedstatus and whether the failed set is otherwise recoverable. - Found: The two sibling events both carry an explicit failed breakdown —
mcp servers addedhasfailed_clients(src/steps/add-mcp-server-to-clients/index.ts:130) andmcp servers removedhasfailed_clients(index.ts:190) — butmcp plugins installed(src/ui/tui/services/mcp-installer.ts:156-166) emits onlyclients,already_installed,attempted, with nofailed. The PR body's own stated contract listsalready_installed/failed/attemptedas the new breakdowns, so this is a genuine omission in changed code, not a misread. - Found: The omission is meaningful —
installPlugins(index.ts:246-265) catches each client's error and pushes aFailed-status result, so theresultsarray feeding this event really can contain failures (e.g.codex.ts:213,claude-code.ts:237plugin-install failures). - Impact: Real but low-impact observability/parity gap: the team can't read plugin-install failure rate directly off this event the way it can for MCP add/remove. However the data isn't lost —
attemptedcontains only genuinely-attempted (supported) plugin clients andclients= Changed ∪ Unchanged, sofailedis derivable asattempted \ clients; failures are also separately captured viaanalytics.captureException, and surfaced to the user in the Done screen. The finding's claim that this 'prevents distinguishing genuine failures from unsupported or skipped clients' is overstated, since unsupported/skipped clients never enterattempted. - Priority: Downgrade to
consider— a genuine, cheap, in-scope inconsistency the author likely intended to include, but telemetry-only with a derivable/duplicated fallback and no user-facing effect, which is below the should_fix bar.
Issue description
The updated analytics event reports successful and already-installed clients plus all attempts, but omits clients with McpClientStatus.Failed. This contradicts the PR's stated analytics contract and prevents distinguishing genuine failures from unsupported or skipped clients.
Suggested fix
Add failed: namesWithStatus(results, McpClientStatus.Failed) to the event properties and cover it with an analytics test containing mixed results.
Prompt to fix with AI (copy-paste)
## Context
@src/ui/tui/services/mcp-installer.ts#L155-164
<issue_description>
The updated analytics event reports successful and already-installed clients plus all attempts, but omits clients with McpClientStatus.Failed. This contradicts the PR's stated analytics contract and prevents distinguishing genuine failures from unsupported or skipped clients.
</issue_description>
<issue_validation>
- **Checked:** All three MCP analytics events added/modified in this PR, plus whether plugin installs can actually yield a `Failed` status and whether the failed set is otherwise recoverable.
- **Found:** The two sibling events both carry an explicit failed breakdown — `mcp servers added` has `failed_clients` (`src/steps/add-mcp-server-to-clients/index.ts:130`) and `mcp servers removed` has `failed_clients` (`index.ts:190`) — but `mcp plugins installed` (`src/ui/tui/services/mcp-installer.ts:156-166`) emits only `clients`, `already_installed`, `attempted`, with no `failed`. The PR body's own stated contract lists `already_installed` / `failed` / `attempted` as the new breakdowns, so this is a genuine omission in changed code, not a misread.
- **Found:** The omission is meaningful — `installPlugins` (`index.ts:246-265`) catches each client's error and pushes a `Failed`-status result, so the `results` array feeding this event really can contain failures (e.g. `codex.ts:213`, `claude-code.ts:237` plugin-install failures).
- **Impact:** Real but low-impact observability/parity gap: the team can't read plugin-install failure rate directly off this event the way it can for MCP add/remove. However the data isn't lost — `attempted` contains only genuinely-attempted (supported) plugin clients and `clients` = Changed ∪ Unchanged, so `failed` is derivable as `attempted \ clients`; failures are also separately captured via `analytics.captureException`, and surfaced to the user in the Done screen. The finding's claim that this 'prevents distinguishing genuine failures from unsupported or skipped clients' is overstated, since unsupported/skipped clients never enter `attempted`.
- **Priority:** Downgrade to `consider` — a genuine, cheap, in-scope inconsistency the author likely intended to include, but telemetry-only with a derivable/duplicated fallback and no user-facing effect, which is below the should_fix bar.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add `failed: namesWithStatus(results, McpClientStatus.Failed)` to the event properties and cover it with an analytics test containing mixed results.
</potential_solution>
…roups Reviewer feedback on #1075: the Done screen auto-dismissed after 2s and whipped past too fast to read when several result groups were stacked. Replace the timeout with an explicit "Press enter to continue" prompt so the user controls when the flow moves on, and add a blank line between result groups so items don't run together. Same treatment for the long detect-error copy, which had a 3s timeout for the same reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Running
wizard mcp addand picking an editor that already has PostHog set up printed a bare "Installation skipped." and exited immediately — no reason given, and the follow-on steps (Slack, suggested prompts) were skipped because the outcome was recorded asFailed.Codex hit this reliably:
codex plugin marketplace addexits non-zero once the marketplace is registered, so the install looked like a failure with an empty result set. Any client whose install genuinely failed produced the same message, so "skipped", "already there" and "broke" were indistinguishable.Changes
installed/already-installed/failed, with a short reason on failures — instead of a list of names that silently dropped everything else.wizard mcp removefirst", "Couldn't install for: … "). The non-interactive path reports the same three outcomes separately.config.toml(and Claude Code checksplugin list) before installing, rather than inferring the no-op from CLI error text; the file-based clients compare the existing entry and skip the write when it's identical.Two things found while tracing the same flow: CLI error text is redacted before it reaches the log/screen/exception reports (the failing command echoes back the
Authorizationheader), and the chosen install mode is passed to the installer instead of read from state set in the same tick — which made a single-editor machine take the MCP-only path after choosing "Install with all features".Analytics keep
clientsmeaning "ended up with it", withalready_installed/failed/attemptedas new breakdowns.Test plan
pnpm build && pnpm test && pnpm fix— 1771 tests pass, no lint errors. New coverage for the already-installed and failure paths in Codex, Claude Code, the shared config-file client, the installer service, and the result helpers (including redaction).Follow-up: the rest of the failure states
An audit of every failure path in both flows found the same class of bug elsewhere, fixed in the second commit:
mcp removeclaimed success unconditionally —removeMCPServerdiscarded each client's result, so every client detected before the attempt was reported as removed even if the removal threw. Removals now carry per-client results and reasons.mcp removeprinted nothing at all — no success, no failure, not even "nothing to remove".--debug.mcp remove --localnever targetedposthog-local— the flag was dropped inMcpInstaller.remove()and ignored by Codex'sremoveServer.mcp removefell back to the non-TTY path on any TUI error, masking real bugs; it now uses the sameisTUIUnavailableguard asmcp add.Known gap, not addressed here: a client that's gated off by platform (e.g. Cursor on Linux) is silently absent from the detected list rather than explained —
isClientSupported()returns a bare boolean with no channel for a reason.Created with PostHog from a Slack thread