fix(desktop): forward MCP UI app data for Codex-driven sessions - #95878
Closed
GeneralistDev wants to merge 27 commits into
Closed
fix(desktop): forward MCP UI app data for Codex-driven sessions#95878GeneralistDev wants to merge 27 commits into
GeneralistDev wants to merge 27 commits into
Conversation
Codex sessions write tool-call metadata on `_meta.posthog`, but the workspace-server only read the legacy `_meta.claudeCode` field when deciding whether a tool call is an MCP call. That gate is what wires a completed MCP tool call to the MCP Apps host, so Codex sessions never triggered it. The Codex mapping layer also only forwarded human-readable text on `tool_call_update`, dropping the tool-call `_meta` and the raw MCP result (`content`/`structuredContent`/`_meta.ui`) that the MCP Apps host needs to render an inline UI resource. Fixes both: read the canonical `_meta.posthog` channel (falling back to the legacy one) via the existing `readAgentToolName`/`readMcpToolName` helpers, and have the Codex mapping carry `_meta` and `rawOutput` on the completed update. Generated-By: PostHog Desktop Task-Id: 7a47c916-8e28-4157-9782-3ed394d68232
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Contributor
|
React Doctor found 1 issue in 1 file · 1 warning. 1 warning
Reviewed by React Doctor for commit |
Contributor
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. 🚨 Comment density — 9% of added code lines are comments (95 of 1080)This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out. Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See Files with the most added comment lines:
This check does not block merging. It updates on every push and clears when the share drops. ℹ️ MCP UI apps size — 33 app(s), 17665.2 KB JSBuilt size of each MCP UI app (
|
HostHog preview —
|
The MCP Apps iframe bridge is torn down and rebuilt whenever the underlying UI resource is refetched (e.g. an MCP tool-discovery refresh). If a tool result was already queued for delivery but the bridge had not yet finished its handshake, teardown discarded the queue — and the "already sent" guard lived in the calling component, so it survived the teardown and blocked any later retry. The chart iframe would mount, then sit on its loading state forever with no error anywhere. Moves the "already sent" tracking into the bridge hook itself, resetting it in the same cleanup that tears the bridge down, so a lost delivery can legitimately retry against the next bridge instead of being silently dropped for good. Also stubs `document.fonts` in the Vitest jsdom setup — the app theme util this area touches calls `document.fonts.check()`, which jsdom doesn't implement, and nothing exercised that code path in tests before now. Generated-By: PostHog Desktop Task-Id: 7a47c916-8e28-4157-9782-3ed394d68232
1 task
…roup groupToolRuns collapses a run of >=2 tool calls into a "Thinking..." marker, which buried an MCP tool's rendered chart behind the collapsed summary instead of showing it as its own row. Excludes MCP tool calls from grouping, mirroring the legacy buildThreadGroups' excludeMcpApps. Generated-By: PostHog Desktop Task-Id: 7a47c916-8e28-4157-9782-3ed394d68232
… call The previous commit excluded any MCP-tagged tool call from grouping, but Codex routes every underlying tool through one inline-exec wrapper, so that pulled every tool call out of grouping and left the thread ungrouped and noisy. Narrows the check to whether the call's resolved result actually carries a UI-app resource, which is also the earliest point a chart could exist. Generated-By: PostHog Desktop Task-Id: 7a47c916-8e28-4157-9782-3ed394d68232
A row that mounts already-completed (a chart pulled out of a collapsed tool-call group, or a scroll-back remount) queues its result via the exec-replay effect, then oninitialized's own remount catch-up sent the same result again with no dedup against that queue. Routes the catch-up through sendResultOnce so both paths share the sentResultForCallRef guard. Generated-By: PostHog Desktop Task-Id: 7a47c916-8e28-4157-9782-3ed394d68232
…o apps A Codex session never finished loading a query-runner chart in the chat, while Claude-based sessions rendered it. The Codex app-server protocol models an absent MCP result optional as a nullable field, so the raw result it hands back carries an explicit structuredContent null where the PostHog MCP server had omitted the key. The app-side zod schema types these fields as optional, which rejects an explicit null, so the app's tool-result notification failed validation and was dropped silently. The app kept waiting for data and stayed on its loading state. Normalize in toCallToolResult, the single boundary every delivery path (exec replay, remount catch-up, subscription) funnels through: drop structuredContent / isError / _meta when null, keep the payload otherwise unchanged. Generated-By: PostHog Desktop Task-Id: c01c9ac0-55de-4805-92fe-8faef02b242a
The Codex app-server models an absent MCP result optional as nullable and serializes it as an explicit JSON null, so the nulls now enter the pipeline once and are removed where they enter, not only at the app bridge. The codex adapter wraps `item.result` with the new shared `omitNullCallToolResultFields` helper, and the app-bridge normalization in `toCallToolResult` now uses the same helper. Stored transcripts and McpAppsService events therefore carry clean payloads, so a future delivery path that skips `toCallToolResult` cannot reintroduce the stuck-chart failure. Generated-By: PostHog Desktop Task-Id: c01c9ac0-55de-4805-92fe-8faef02b242a
A reply to a PostHog MCP query now shows two charts: the interactive chart the MCP app renders for the tool result, and the model's own embedded hogql chart. The rich-output prompt tells the model to render every numeric answer as a hogql chart, and it follows that instruction even when the result already rendered as an interactive chart. Add one rule to the shared prompt, which every harness receives with its session prompt: a PostHog MCP query call already renders its result as an interactive chart, so the reply states the conclusion in text and does not embed the same data again. Generated-By: PostHog Desktop Task-Id: c01c9ac0-55de-4805-92fe-8faef02b242a
…result
The shared rich-output prompt rule ("do not duplicate an MCP chart with a
hogql one") stopped GPT/Codex sessions from re-embedding the chart, but a
Claude Sonnet session still showed two charts: the interactive chart the
MCP app renders for query-trends, and the model's own <hogql> chart under
it. A rule in the session prompt proved too far from the data for Sonnet
to honor at reply time.
Append a note to the tool result text itself, at the point where the
reply gets written. It fires only on the exec path for clients that
render UI apps (PostHog Desktop, Claude Code inside it, Cowork) - a CLI
client sees no view, and its model may legitimately re-present the data,
so it gets no note. output_format=json keeps the text machine-parseable
with no footer.
estimateResponseTokens keyed off the exact STRUCTURED_CONTENT_ONLY_TEXT
pointer to decide the structured payload is what gets counted; the note
now follows the pointer, so the check matches the prefix instead.
Generated-By: PostHog Desktop
Task-Id: 2e57bf95-62bd-4353-adba-6a584f3267ce
A local MCP build with POSTHOG_UI_APPS_TOKEN set rendered a blank app iframe in PostHog Desktop. posthog-js-lite 4.11.0 reads window.localStorage in its constructor without a guard; MCP app iframes are sandboxed without allow-same-origin, so the property read throws SecurityError, uncaught, and kills the whole app. Production renders only because its build leaves the token unset, so analytics never initializes. Guard initPostHog with try/catch: analytics is optional, and a client that cannot initialize must not take the app down with it. The capture helpers already no-op on a null client, so a failed init disables analytics and nothing else. No new test: it would have to mock the posthog-js-lite constructor to assert the catch runs, which tests that a try/catch exists rather than any behavior. The unit suites still pass. Generated-By: PostHog Desktop Task-Id: 2e57bf95-62bd-4353-adba-6a584f3267ce
GeneralistDev
marked this pull request as ready for review
September 8, 2026 09:09
Contributor
Prompt To Fix All With AI### Issue 1
services/mcp/src/lib/build-tool-result.ts:110
**Render note tokens omitted**
When an inline UI response contains `structuredContent`, this branch estimates only the structured payload and ignores the render note appended to the delivered text. As a result, every structured-only UI response underreports output-token usage. The estimate should include the footer while still avoiding the duplicated pointer text.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(mcp): keep a UI app alive when posth..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Generated-By: PostHog Desktop Task-Id: 66100a80-baa2-4414-94ab-b96105ea756f
…e PR Generated-By: PostHog Desktop Task-Id: 66100a80-baa2-4414-94ab-b96105ea756f
Cut the diff's comment lines roughly in half. Dropped narration that restated the code or the assertions below it; kept the non-obvious whys (zod rejects explicit nulls, group bodies stay unmounted, bridge-scoped dedup) in shorter form. Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
GeneralistDev
commented
Sep 8, 2026
| - Inline reference: \`<kind id="...">short human label</kind>\` inside a sentence, e.g. \`The <insight id="9pQx3">checkout funnel</insight> dropped after <flag id="42">new-checkout-flow</flag> rolled out.\` Kinds: insight, dashboard, error, replay, flag, experiment, survey, ticket, report, trace, eval, event, cohort, action, person. Use the object's id (insights: the short id; feature flags: the numeric id, falling back to the key; Inbox reports: the report uuid; persons: the uuid). It renders as a chip with a live hover preview that opens the object in PostHog. | ||
| - Inline SQL: \`<hogql label="signups today">SELECT count() FROM events WHERE ...</hogql>\` - the SQL is the tag body, the label is what the sentence shows. Hovering runs the query live; clicking opens the SQL editor. | ||
| - Full-size chart, for any numeric or time-series answer (always prefer this over a markdown table): a saved insight \`<insight id="9pQx3" display="block"/>\` or a query \`<hogql display="block" title="Daily active users, last 7 days" caption="optional context">SELECT ...</hogql>\`. The chart executes live on every view. Include the time range in the title, and keep blank lines out of the SQL body. | ||
| - A PostHog MCP query call already renders its result as an interactive chart in the conversation. When your answer comes from such a call, do not embed the same data again as a \`<hogql>\` chart; write the conclusion in text and let the rendered chart carry the data. |
Member
Author
There was a problem hiding this comment.
This was critical for both codex and claude code, both of which had the tendency to use hogql even when the interactive chart was already shown to the user.
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Generated-By: PostHog Desktop Task-Id: b90d65d3-3dd6-44f0-ba05-279d39aa40fa
Contributor
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 1 consider. Published 2 findings (view the review). |
A query tool result reaches a Pi session with its UI payload on `_meta`, but three gaps stop the chart from rendering: the harness MCP tool keeps only the content blocks and drops `structuredContent` and `_meta`, the proxy tool's details never classify the call as MCP, and no Pi code path registers MCP server configs with the apps service. The server already tells the model that the interactive view shows the result, so the model stops drawing its own chart and the user sees no chart at all. - The harness MCP tool now carries `structuredContent` and `_meta` on the tool result's details, out of the model context, on both the proxy and direct call paths. - The Pi translator classifies those calls as MCP and lands the full result on `rawOutput`, which the apps host resolves. - The Pi client factory registers the session's MCP servers with the apps service and runs discovery, like AgentService does for Claude and Codex. - Exec tool rows read the label from the arguments the `mcp` proxy tool wraps. Generated-By: PostHog Desktop Task-Id: 7d773290-e531-4e36-9806-50f3a5db6e3f
Contributor
|
PostHog Review 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}
| @@ -153,7 +141,8 @@ export function McpAppHost({ | |||
| onData: (event) => { | |||
Contributor
There was a problem hiding this comment.
Non-exec app results bypass call-level deduplication
Why we think it's a valid issue
- Checked: both branches of the
onToolResultcallback, the subscription's server-side filter, the net PR diff for this file and foruseAppBridge.ts, the app-side result handler, and what the thread renders per tool call. - Found: the filter asymmetry is real and confirmed. The exec branch drops foreign events with
if (event.toolCallId !== toolCall.toolCallId) return;(McpAppHost.tsx:143); the non-exec branch atMcpAppHost.tsx:148-154sends whatever arrives. The subscription itself is scoped by tool key only — the router yields every event whereevent.toolKey === targetToolKey(products/desktop/packages/host-router/src/routers/mcp-apps.router.ts:89-101), andMcpAppsService.notifyToolResultemits app-wide (products/desktop/packages/core/src/mcp-apps/mcp-apps.ts:653-672). So a second call of the same non-exec UI tool delivers its result into an earlier, still-mounted app, and that app re-renders with another call's data (services/mcp/src/ui-apps/hooks/useToolResult.ts:195-224callssetDataon every result). - Found: the duplicate-delivery half is benign. Both sends carry identical bytes — the subscription path uses
event.result, which isupdate.rawOutput(products/desktop/packages/workspace-server/src/services/agent/agent.ts:2220-2225), and the remount replay uses the sametc.rawOutput(useAppBridge.ts:313-320) — and the app's handler ignoresisError, re-parsesstructuredContent, and sets the same state. The only cost is a repeatedmcp_ui_app_tool_resultevent. - Found: nothing regressed in this branch. The net diff touches only the exec branch and moves the dedupe helper into the hook;
McpAppHost.tsx:148-154is unchanged context, and theoninitializedremount replay predates the PR — it previously calledbridge.sendToolResultdirectly with no dedupe at all. What the change does add israwOutputon Codex MCP items (mapping.ts:565-567), which lets that replay fire for Codex sessions too. - Found: reachability of the wrong-data case is narrow. PostHog's own desktop surface routes through the exec tool, which already has the guard, and back-to-back non-exec tool calls fold into a collapsed group whose body stays unmounted —
hasUiAppResultonly un-folds result-embedded URIs (ChatThread.tsx:224-235), so it does not exempt tool-key-resolved apps. It needs a second call of the same non-exec UI tool while an earlier host is still mounted. - Impact: an app can render a different call's result, which is wrong data on screen. The fix is the one-line guard the exec branch already uses, in a file this PR edits.
- Priority: lowered from
must_fix. The harmful half predates this diff and needs an uncommon sequence, and the newly enabled half (double delivery of an identical payload) has no observable effect.
Issue description
Codex now stores rawOutput for every MCP call. A registered app can receive both this stored result and its subscription event before initialization. The non-exec branch bypasses sendResultOnce, so both results reach the app. It also accepts events from other calls that use the same tool key.
Suggested fix
Check event.toolCallId for every result. Send each matching event through sendResultOnce(event.toolCallId, event.result). Add tests for pre-initialization delivery and concurrent calls.
Prompt to fix with AI (copy-paste)
## Context
@products/desktop/packages/ui/src/features/mcp-apps/components/McpAppHost.tsx#L141-154
<issue_description>
Codex now stores rawOutput for every MCP call. A registered app can receive both this stored result and its subscription event before initialization. The non-exec branch bypasses sendResultOnce, so both results reach the app. It also accepts events from other calls that use the same tool key.
</issue_description>
<issue_validation>
- **Checked:** both branches of the `onToolResult` callback, the subscription's server-side filter, the net PR diff for this file and for `useAppBridge.ts`, the app-side result handler, and what the thread renders per tool call.
- **Found:** the filter asymmetry is real and confirmed. The exec branch drops foreign events with `if (event.toolCallId !== toolCall.toolCallId) return;` (`McpAppHost.tsx:143`); the non-exec branch at `McpAppHost.tsx:148-154` sends whatever arrives. The subscription itself is scoped by tool key only — the router yields every event where `event.toolKey === targetToolKey` (`products/desktop/packages/host-router/src/routers/mcp-apps.router.ts:89-101`), and `McpAppsService.notifyToolResult` emits app-wide (`products/desktop/packages/core/src/mcp-apps/mcp-apps.ts:653-672`). So a second call of the same non-exec UI tool delivers its result into an earlier, still-mounted app, and that app re-renders with another call's data (`services/mcp/src/ui-apps/hooks/useToolResult.ts:195-224` calls `setData` on every result).
- **Found:** the duplicate-delivery half is benign. Both sends carry identical bytes — the subscription path uses `event.result`, which is `update.rawOutput` (`products/desktop/packages/workspace-server/src/services/agent/agent.ts:2220-2225`), and the remount replay uses the same `tc.rawOutput` (`useAppBridge.ts:313-320`) — and the app's handler ignores `isError`, re-parses `structuredContent`, and sets the same state. The only cost is a repeated `mcp_ui_app_tool_result` event.
- **Found:** nothing regressed in this branch. The net diff touches only the exec branch and moves the dedupe helper into the hook; `McpAppHost.tsx:148-154` is unchanged context, and the `oninitialized` remount replay predates the PR — it previously called `bridge.sendToolResult` directly with no dedupe at all. What the change does add is `rawOutput` on Codex MCP items (`mapping.ts:565-567`), which lets that replay fire for Codex sessions too.
- **Found:** reachability of the wrong-data case is narrow. PostHog's own desktop surface routes through the exec tool, which already has the guard, and back-to-back non-exec tool calls fold into a collapsed group whose body stays unmounted — `hasUiAppResult` only un-folds result-embedded URIs (`ChatThread.tsx:224-235`), so it does not exempt tool-key-resolved apps. It needs a second call of the same non-exec UI tool while an earlier host is still mounted.
- **Impact:** an app can render a different call's result, which is wrong data on screen. The fix is the one-line guard the exec branch already uses, in a file this PR edits.
- **Priority:** lowered from `must_fix`. The harmful half predates this diff and needs an uncommon sequence, and the newly enabled half (double delivery of an identical payload) has no observable effect.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Check event.toolCallId for every result. Send each matching event through sendResultOnce(event.toolCallId, event.result). Add tests for pre-initialization delivery and concurrent calls.
</potential_solution>
A model that passes a tool's arguments straight to the `mcp` proxy tool, without the `tool` name, got the generic usage line. The line names no tool, so the model burns retries guessing at the double wrapping. A GLM session hit exactly this and fell back to a raw hogql chart. Match the stray argument keys against each tool's required parameter names, from live registrations first and then the on-disk tool cache, and name the tool in the hint. One retry assembles the correct call. Required parameter names now ride on the bridge's tool metadata and the cache entries. Old cache entries lack the field and stay unmatched until the next refresh rewrites them, so the hint degrades to the generic line. Generated-By: PostHog Desktop Task-Id: 7d773290-e531-4e36-9806-50f3a5db6e3f
The rich-output prompt claimed every PostHog MCP query call already renders its result as an interactive chart. Only tools with a UI app do: query-trends and the other insight wrappers, not execute-sql or query-logs. On those two paths the bullet told the model to skip the hogql chart the bullet above it requires, so a user got a text-only answer where they previously got a live chart. The MCP server already emits the exact signal: UI_APP_RENDER_NOTE rides the tool result only when the tool has a resourceUri and the client is a UI-app host. Anchor the skip rule to that note instead of to the tool call, so the model skips the chart only when a view will actually render. Generated-By: PostHog Desktop Task-Id: ef6db808-523a-4319-b0a6-d4897e83f41d
Generated-By: PostHog Desktop Task-Id: ef6db808-523a-4319-b0a6-d4897e83f41d
#96406 re-pinned the bundled agents (Explore to Luna, Plan to Sol, General to Terra but left the tests asserting the old values, so desktop CI failed on master and on every PR merged with it. Update the assertions to the new intended models. Generated-By: PostHog Desktop Task-Id: ef6db808-523a-4319-b0a6-d4897e83f41d EOF )
…hog/fix-codex-mcp-apps-rendering # Conflicts: # products/desktop/packages/harness/src/extensions/orchestration/agents.test.ts
This was referenced Sep 8, 2026
Member
Author
|
Split into a 3-layer stack for review. No code changed: the combined tree of the three layers matches this branch exactly.
|
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
1 participant
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
People who query PostHog from Desktop can see plain text, a loading chart, a hidden chart, duplicate charts, or no chart at all, instead of one interactive result.
Codex, Claude, and Pi sessions each fail at a different point in the MCP app flow. In Pi sessions the model also believes the interactive view already rendered, so it stops drawing its own chart and the user sees no chart at all.
Changes
structuredContentand_metaon the tool result, out of the model context.mcpproxy tool wraps.mcpcall that passes a tool's arguments without thetoolname now names the tool they match. The model corrects the double wrap in one retry instead of guessing.Screenshots
Claude Sonnet renders one interactive chart.
GPT 5.6 Luna renders one interactive chart.
GLM-5.3 using Pi as the harness renders one interactive chart.
How did you test this code?
structuredContentand_metareach the tool result details on both call paths, through the real SDK client and server. They catch the passthrough being dropped again.rawOutput. They catch Pi sessions going back to plain text.mcpproxy tool wraps.👉 See the PostHog coding conventions.
Automatic notifications
Docs update
None. This internal fix does not change a documented setting or workflow.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
PostHog Desktop and command-line development tools supported the diagnosis and implementation.
Skills invoked:
/writing-tests,/writing-ui-components,/writing-pr-descriptions,/writing-simplified-technical-english,/unslop, and/asd-ste100.Created with PostHog Desktop