feat(cli): cross-flavor inline image and video display via MCP and ACP#958
feat(cli): cross-flavor inline image and video display via MCP and ACP#958heavygee wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Findings
- [Major] ACP inline media can be emitted before preceding assistant text. The new image branch flushes reasoning but leaves
bufferedTextopen, whileemitGeneratedImageFromAcpContent()emits asynchronously. For a normal ACP sequence like text chunk -> image block -> turn drain, the generated-image message can reach the web UI before the text that came first in the stream. Evidence:cli/src/agent/backends/acp/AcpMessageHandler.ts:559.
Questions
- None.
Summary
- Review mode: initial
- One ordering regression found in the latest diff. Residual risk: media fetch/cache behavior was reviewed statically only.
Testing
- Not run (automation). Suggested coverage: ACP handler test that sends a text
agentMessageChunk, then an imageagentMessageChunk, then drains, and asserts text precedesgenerated_image.
Flush buffered assistant text before async generated_image emit from ACP image blocks (PR tiann#958 review Major). Add optional source metadata on generated-image wire messages (ingress, flavor, toolCallId, toolName) for MCP, ACP, and Codex tool-result paths. Seeds artifact-event follow-up tiann#966. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Findings
- [Major] Generated media cards can keep rendering a stale blob after
imageIdchanges. - [Major] ACP image registration can let later updates render before the image block that preceded them.
Summary
Review mode: initial
Latest PR diff reviewed in full. The main residual risk is around generated media lifecycle/order because the new tests cover happy-path conversion and MIME helpers, but not component prop changes or ACP image blocks followed by later updates.
Testing
Not run (automation)
HAPI Bot
…tale blobs Queue ACP session updates and await async image registration before later events; clear GeneratedImageCard blob state when imageId changes. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Findings
- [Major] Await ACP updates that arrive during the late-drain window before emitting
turn_complete.cli/src/agent/backends/acp/AcpSdkBackend.ts:429
Summary
Review mode: follow-up after new commits
The previous stale-blob and initial ACP ordering findings are addressed. One ordering gap remains in the follow-up fix: late ACP image/file-backed updates can still be queued during drainLateBuffers() and emitted after turn_complete.
Testing
Not run (automation). Suggested: ACP prompt lifecycle test where a local-path image update arrives during drainLateBuffers() and must appear before turn_complete.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Minor] AVIF files are now routed to
display_video, so the helper rejects a supported image format.scripts/tooling/hapi-display-image.mjs:32
Suggested fix:if (head.length >= 12 && head.subarray(4, 8).toString('ascii') === 'ftyp') { const brand = head.subarray(8, 12).toString('ascii') return brand === 'avif' || brand === 'avis' ? 'display_image' : 'display_video' }
Summary
Review mode: follow-up after new commits
One minor issue found in the helper script. The previous ACP ordering finding is addressed by the added post-drainLateBuffers() queue drain.
Testing
Not run (automation). Suggested: add a script/media sniffing test or shared helper coverage for AVIF vs MP4 ftyp routing.
HAPI Bot
There was a problem hiding this comment.
Findings
No issues found.
Summary
Review mode: follow-up after new commits
The follow-up commit addresses the previous AVIF ftyp routing finding. I reviewed the full current diff across MCP/ACP media registration, generated-image RPC serving, web rendering, and helper tooling; no new high-confidence correctness, security, data-loss, or regression findings found. Residual risk: I did not run the full Bun test suite in this automation environment.
Testing
Not run (automation)
HAPI Bot
There was a problem hiding this comment.
Findings
- [Minor]
HAPI_SESSION_IDshorthand drops the optional title — the new usage text advertisesHAPI_SESSION_ID=<uuid> hapi-display-image.mjs <media-path> [title], but when a title is presentprocess.argv[3]is treated asimagePath, so the later file check runs against the title string and exits before calling MCP. Evidence:scripts/tooling/hapi-display-image.mjs:27.
Suggested fix:if (envSessionPrefix && sessionArg && existsSync(sessionArg) && lstatSync(sessionArg).isFile()) { imagePath = sessionArg title = process.argv[3] sessionArg = envSessionPrefix } else if (!sessionArg && envSessionPrefix) { sessionArg = envSessionPrefix }
Summary
Review mode: follow-up after new commits
Reviewed the full current PR diff, with prior bot review and the 67ca0b80...e3ab425 compare as follow-up context. One helper-script argument parsing regression found. Residual risk: full Bun test suite not run in this automation pass.
Testing
Not run (automation)
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Generated media fetch bypasses the immutable browser cache —
cache: 'no-store'prevents the browser from storing or reusing the response, so every remount of an inline image/video goes back through the hub and CLI RPC despite the route explicitly returningCache-Control: immutableand having a regression test for avoiding that round-trip. Evidence:web/src/api/client.ts:333.
Suggested fix:const res = await fetch(this.buildUrl(`/api/sessions/${encodeURIComponent(sessionId)}/generated-images/${encodeURIComponent(imageId)}`), { headers })
Summary
Review mode: follow-up after new commits
Reviewed the full current PR diff with the previous bot review and follow-up compare as context. The prior HAPI_SESSION_ID title parsing issue is fixed. One cache regression remains in the generated media client path.
Testing
Not run (automation)
HAPI Bot
There was a problem hiding this comment.
Findings
None.
Summary
Review mode: follow-up after new commits
Reviewed the full current PR diff, using the previous HAPI Bot review and follow-up compare as context. The prior generated-media cache issue is addressed in the latest head. No blocking or high-confidence correctness/security regressions found in the changed lines. Residual risk: I did not execute PR code, so runtime/browser behavior for the new inline video path still depends on the workflow/manual test results.
Testing
Not run (automation)
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Avoid restoring stale Cursor MCP config wholesale —
installCursorMcpOverlay()snapshots the entire.cursor/mcp.jsonbefore adding HAPI, then cleanup writes that old snapshot back. Any user/Cursor edits made while the remote session is running are silently lost on cleanup, which is a user config data-loss regression. Evidence:cli/src/cursor/utils/cursorMcpOverlay.ts:92
Suggested fix:const installedEntry = { command: bridge.command, args: [...bridge.args], }; config.mcpServers[CURSOR_HAPI_MCP_SERVER_ID] = installedEntry; // cleanup: re-read current file and only undo this overlay entry. const current = readMcpJson(mcpJsonPath); const currentEntry = current.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID]; if (JSON.stringify(currentEntry) !== JSON.stringify(installedEntry)) { return; } delete current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID]; const previous = previousContent ? parseMcpJson(previousContent) : null; const previousEntry = previous?.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID]; if (previousEntry) { current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID] = previousEntry; } writeMcpJson(mcpJsonPath, current);
Summary
Review mode: follow-up after new commits
Reviewed the full current PR diff, using the previous HAPI Bot review and compare diff as context. Found one Major issue in the new Cursor MCP overlay cleanup path. Residual risk: runtime browser/media behavior was not executed in this review.
Testing
Not run (automation)
HAPI Bot
| return { | ||
| cleanup: () => { | ||
| try { | ||
| if (previousContent !== null) { |
There was a problem hiding this comment.
[MAJOR] This cleanup path restores the full previousContent snapshot captured before HAPI was injected. If the user or Cursor updates .cursor/mcp.json while the session is running, those unrelated changes get overwritten when the launcher exits. Re-read the current file during cleanup and remove/restore only the hapi entry that this overlay installed.
Suggested fix:
const installedEntry = {
command: bridge.command,
args: [...bridge.args],
};
config.mcpServers[CURSOR_HAPI_MCP_SERVER_ID] = installedEntry;
// cleanup: re-read current file and only undo this overlay entry.
const current = readMcpJson(mcpJsonPath);
const currentEntry = current.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID];
if (JSON.stringify(currentEntry) !== JSON.stringify(installedEntry)) {
return;
}
delete current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID];
const previous = previousContent ? parseMcpJson(previousContent) : null;
const previousEntry = previous?.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID];
if (previousEntry) {
current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID] = previousEntry;
}
writeMcpJson(mcpJsonPath, current);Share display_image prompt across MCP-bridge flavors (Cursor, Gemini, Kimi, Codex, Claude, OpenCode), auto-approve the tool in buildHapiMcpBridge, handle ACP image content blocks, and harden generated-image registration with content sniffing. Closes tiann#956 Co-authored-by: Cursor <cursoragent@cursor.com>
Keep object URLs stable across refetch, upscale tiny inline images, fetch generated-image bytes with cache no-store (avoid empty 304 bodies), and load hapiMcpUrl from per-session API in hapi-display-image tooling. Co-authored-by: Cursor <cursoragent@cursor.com>
Add display_video alongside display_image, video MIME sniffing with avif guard, web GeneratedImageCard video player, and hapi-display-image auto-routing. Co-authored-by: Cursor <cursoragent@cursor.com>
Share display_video prompts across MCP-bridge flavors, auto-approve the tool, register mp4/webm via path sniffing, render inline video in web on the existing generated-image RPC path, and restore robust media card fetch. Co-authored-by: Cursor <cursoragent@cursor.com>
Bun hoists @modelcontextprotocol/sdk to the repo root; importing via cli/node_modules broke the dogfood script in worktrees. Co-authored-by: Cursor <cursoragent@cursor.com>
Flush buffered assistant text before async generated_image emit from ACP image blocks (PR tiann#958 review Major). Add optional source metadata on generated-image wire messages (ingress, flavor, toolCallId, toolName) for MCP, ACP, and Codex tool-result paths. Seeds artifact-event follow-up tiann#966. Co-authored-by: Cursor <cursoragent@cursor.com>
…tale blobs Queue ACP session updates and await async image registration before later events; clear GeneratedImageCard blob state when imageId changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Straggler session/update during drainLateBuffers can queue async image registration; re-await sessionUpdateQueue so generated_image is not emitted after turn_complete. Co-authored-by: Cursor <cursoragent@cursor.com>
Match server-side detectImageMimeType so .avif files are not sent to display_video and rejected as unsupported video. Co-authored-by: Cursor <cursoragent@cursor.com>
- hapi doctor inline-media: probe bridges, print per-session inline commands - Expose hapiMcpUrl on session list summaries (stops false "no MCP" scans) - Helper script: match cursorSessionId prefixes; HAPI_SESSION_ID path-only mode - ACP bridge prompt: shell fallback + HAPI session id vs agent id rule Co-authored-by: Cursor <cursoragent@cursor.com>
When env session id is set, treat argv[2] as media path even if argv[3] is title (drop !imagePath guard that mis-assigned title as imagePath). Co-authored-by: Cursor <cursoragent@cursor.com>
Drop cache: no-store on generated-image fetch so browser can reuse hub immutable responses; on 304 re-read via force-cache (tiann#927, PR review). Co-authored-by: Cursor <cursoragent@cursor.com>
Cursor ACP ignores session/new mcpServers. Write .cursor/mcp.json and run agent mcp enable hapi instead. Remove HAPI_MCP_BRIDGE_PROMPT from user turns on ACP remotes; enrich MCP tool descriptions for discovery. Co-authored-by: Cursor <cursoragent@cursor.com>
b8e9dd2 to
7337eae
Compare
There was a problem hiding this comment.
Findings
- [Major] Avoid restoring stale Cursor MCP config wholesale —
installCursorMcpOverlay()snapshots the whole.cursor/mcp.jsonbefore adding HAPI and cleanup writes that old snapshot back. Any Cursor/user edits made while the session is running are silently lost when the launcher exits, so this is a user config data-loss regression. Evidence:cli/src/cursor/utils/cursorMcpOverlay.ts:92
Suggested fix:const installedEntry = { command: bridge.command, args: [...bridge.args], }; config.mcpServers[CURSOR_HAPI_MCP_SERVER_ID] = installedEntry; // cleanup: re-read current file and only undo this overlay entry. const current = readMcpJson(mcpJsonPath); const currentEntry = current.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID]; if (JSON.stringify(currentEntry) !== JSON.stringify(installedEntry)) { return; } delete current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID]; const previous = previousContent ? parseMcpJson(previousContent) : null; const previousEntry = previous?.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID]; if (previousEntry) { current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID] = previousEntry; } writeMcpJson(mcpJsonPath, current);
Summary
Review mode: follow-up after new commits
Reviewed the full current PR diff, using the previous HAPI Bot review and compare diff as context. The prior Cursor MCP cleanup data-loss issue is still present at the current head. Residual risk: runtime browser/media behavior was not executed in this review.
Testing
Not run (automation)
HAPI Bot
| return { | ||
| cleanup: () => { | ||
| try { | ||
| if (previousContent !== null) { |
There was a problem hiding this comment.
[MAJOR] This cleanup path restores the full previousContent snapshot captured before HAPI was injected. If the user or Cursor updates .cursor/mcp.json while the session is running, those unrelated changes get overwritten when the launcher exits. Re-read the current file during cleanup and remove/restore only the hapi entry that this overlay installed.
Suggested fix:
const installedEntry = {
command: bridge.command,
args: [...bridge.args],
};
config.mcpServers[CURSOR_HAPI_MCP_SERVER_ID] = installedEntry;
// cleanup: re-read current file and only undo this overlay entry.
const current = readMcpJson(mcpJsonPath);
const currentEntry = current.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID];
if (JSON.stringify(currentEntry) !== JSON.stringify(installedEntry)) {
return;
}
delete current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID];
const previous = previousContent ? parseMcpJson(previousContent) : null;
const previousEntry = previous?.mcpServers?.[CURSOR_HAPI_MCP_SERVER_ID];
if (previousEntry) {
current.mcpServers![CURSOR_HAPI_MCP_SERVER_ID] = previousEntry;
}
writeMcpJson(mcpJsonPath, current);
Summary
Cross-flavor inline image and video in HAPI web chat (not Cursor IDE composer):
display_imageanddisplay_videoinstartHappyServer; stdio bridge forwards both;buildHapiMcpBridgeauto-approves both tools.displayImagePrompt/display_videoexports for Claude, Codex, andHAPI_MCP_BRIDGE_PROMPT(Cursor, Gemini, Kimi, OpenCode first-prompt injection).generated-imageagent messages; generated media stored in CLI memory and served via hubGET /api/sessions/:id/generated-images/:imageId(images and video share this route).GeneratedImageCardrenders<img>(with tiny-image upscale + stable blob fetch) or<video controls>whenmimeTypeisvideo/*.hapi-display-image.mjsroutes mp4/webm todisplay_video(absolute paths); loadshapiMcpUrlfrom per-session GET.Test plan
bun typecheckgeneratedImages.test.ts,buildHapiMcpBridge.test.ts,codexMcpConfig.test.ts,AcpMessageHandler.test.ts,messageConverter.test.tsgeneratedInlineMedia.test.tsdisplay_imagePNG anddisplay_videoMP4 on Cursor session withhapiMcpUrl; inline cards in HAPI webIssues
Closes #956