APP-2527 Phase 2: MCP data pipeline

[cephalonaut]

What

Thread the structured serde_json::Value MCP tool call request through to RequestedCommandView and normalize CallMCPToolResult into a renderable form. No visible UI change — the old Text render path stays active.

• McpRequest struct: holds name: String and args: serde_json::Value for tree rendering
• New fields on RequestedCommandView: mcp_request: Option<McpRequest> and mcp_tree_state: JsonTreeState (path-keyed expansion state for both request and response trees)
• New action variants: ToggleJsonNode { path } and ToggleJsonString { path } on RequestedCommandViewAction, handled by delegating to mcp_tree_state
• McpRenderable enum and mcp_result_to_renderable(): normalizes CallMCPToolResult — prefers structured_content, falls back to parsing text content as JSON, then wraps plain text as a JSON String value; Error and Cancelled map directly
• update_mcp_request(): sets mcp_request on the view from stream updates
• block.rs: extends handle_mcp_tool_stream_update to accept and propagate the coerced display_input and name, calling update_mcp_request on the view

Why

Phase 2 of APP-2527. Establishes the data pipeline so Phase 3 can wire render_json_tree into the MCP tool call detail body without any additional data-layer work.

Agent Mode

• This PR was created by Warp Agent Mode

Testing

Added 5 unit tests for mcp_result_to_renderable in json_tree_tests.rs:

• Success with structured_content → Tree(value)
• Success with JSON text content → Tree(parsed_value)
• Success with non-JSON text → Tree(String(text))
• Error → Error(msg)
• Cancelled → Cancelled

All 31 tests in the json_tree_tests suite pass. No existing tests regressed.

---

Conversation: https://staging.warp.dev/conversation/ebf3f6a3-c6f6-4fff-9aca-5e4582527167
Run: https://oz.staging.warp.dev/runs/019ede47-c45b-78a5-ac6e-7fd91b3c6404
This PR was generated with Oz.

[cephalonaut]

Phase 2 Review — APP-2527

Verdict: MINOR — all blocking Phase 2 requirements are met; four minor items must be resolved before Phase 3 launches.

---

✅ Requirements satisfied

• McpRequest { name: String, args: serde_json::Value } struct defined and used.
• RequestedCommandView carries mcp_request: Option<McpRequest> and mcp_tree_state: JsonTreeState.
• ToggleJsonNode { path: Vec<PathSegment> } and ToggleJsonString { path: Vec<PathSegment> } action variants are present and fully handled in handle_action with the correct toggle / toggle_string + ctx.notify() calls.
• JsonTreeState and PathSegment are imported from Phase 1 (crate::ui_components::json_tree), not redefined.
• handle_mcp_tool_stream_update signature extended correctly; the command_text string is built identically to before.
• mcp_result_to_renderable logic is correct across all five cases: structured_content → JSON text → String fallback → Error → Cancelled.
• Five unit tests for mcp_result_to_renderable are present and meaningful.
• The old should_render_mcp_content / content_text render path is untouched.
• Code comments are free of spec/process references and explain the "why" locally.

---

🔶 Minor items (must fix before Phase 3)

M1 — Carry-over from Phase 1: file-level doc comment in json_tree_tests.rs

app/src/ui_components/json_tree_tests.rs line 1:

//! Pure-logic unit tests for the `json_tree` component (Phase 1, APP-2527).

Remove the phase/ticket reference. Reword to something like:

//! Pure-logic unit tests for the `json_tree` component.

M2 — Carry-over from Phase 1: no tests for toggle_string / is_string_expanded

JsonTreeState::toggle_string and is_string_expanded are exercised in the Phase 2 action handler but have no unit tests. Add at least one test each to json_tree_tests.rs (e.g., verify collapsed-by-default, toggling to true, toggling back to false).

M3 — Depth calculation in ToggleJsonNode will be wrong if Phase 3 uses synthetic namespace segments

RequestedCommandViewAction::ToggleJsonNode { path } => {
    let depth = path.len();         // ← inferred from path length
    self.mcp_tree_state.toggle(path.clone(), depth);

The spec says mcp_tree_state uses a synthetic prefix segment (PathSegment::Key("__request__") / PathSegment::Key("__response__")) to namespace request and response paths. If Phase 3 follows that scheme, every path will include this extra leading segment, making path.len() equal to render_depth + 1. That shifts the root node from depth 0 (expanded by default) to depth 1 (collapsed by default), breaking the initial-open UX.

Fix before Phase 3 by one of:

• Add depth: usize to the ToggleJsonNode action and have the Phase 3 toggle callback supply the render-time depth directly.
• Or commit to Phase 3 using separate JsonTreeState instances for request and response (no namespace segments), in which case path.len() == render_depth is guaranteed and no action change is needed.

Either approach is valid; the spec should be updated to record the decision.

M4 — Unnecessary .clone() in both action handlers

path is moved out of the enum variant by destructuring, so the clones below are redundant:

// ToggleJsonNode
let depth = path.len();
self.mcp_tree_state.toggle(path.clone(), depth);  // .clone() not needed

// ToggleJsonString
self.mcp_tree_state.toggle_string(path.clone());  // .clone() not needed

Change to pass path directly (the .len() call does not consume it):

let depth = path.len();
self.mcp_tree_state.toggle(path, depth);

self.mcp_tree_state.toggle_string(path);

Clippy (-D warnings) will catch this — it must be clean before the Phase 3 PR is opened.

[cephalonaut]

Phase 2 Re-Review — APP-2527

Verdict: MINOR — M1, M2, M3 fully resolved. M4 still open (redundant clone was relocated, not removed).

---

✅ M1 — Fixed

File-level doc comment in json_tree_tests.rs now reads:

//! Pure-logic unit tests for the `json_tree` component.

No phase/ticket reference remains.

---

✅ M2 — Fixed

Three well-designed tests added (string_collapsed_by_default, toggle_string_expands_then_collapses, toggle_string_is_independent_of_node_expansion). Cover the default state, round-trip toggle, and independence from node-expansion state.

---

✅ M3 — Fixed

Split into mcp_request_tree_state + mcp_response_tree_state. McpTree { Request, Response } enum correctly added. Both action variants carry tree: McpTree and dispatch to the correct state. depth = path.len() is now accurate because no synthetic namespace segments are in the paths. The PartialEq + Eq derives were correctly removed from RequestedCommandViewAction (the Action trait only requires Any + Debug + Send + Sync; no application code compares actions with ==).

---

🔶 M4 — Still open

The clone was moved from the call site to a let rebinding, but not eliminated:

// ToggleJsonNode handler
RequestedCommandViewAction::ToggleJsonNode { path, tree } => {
    let depth = path.len();
    let path = path.clone();     // ← still a redundant clone
    match tree {
        McpTree::Request => self.mcp_request_tree_state.toggle(path, depth),
        McpTree::Response => self.mcp_response_tree_state.toggle(path, depth),
    }
    ctx.notify();
}

// ToggleJsonString handler
RequestedCommandViewAction::ToggleJsonString { path, tree } => {
    let path = path.clone();     // ← still a redundant clone
    match tree {
        McpTree::Request => self.mcp_request_tree_state.toggle_string(path),
        McpTree::Response => self.mcp_response_tree_state.toggle_string(path),
    }
    ctx.notify();
}

The original path is not used after the clone is made, so clippy::redundant_clone still fires. Because Rust's borrow checker allows the same variable to appear in multiple match arms (only one arm executes, so there is no double-move), path can be moved directly without cloning:

// ToggleJsonNode — fixed
RequestedCommandViewAction::ToggleJsonNode { path, tree } => {
    let depth = path.len();
    match tree {
        McpTree::Request => self.mcp_request_tree_state.toggle(path, depth),
        McpTree::Response => self.mcp_response_tree_state.toggle(path, depth),
    }
    ctx.notify();
}

// ToggleJsonString — fixed
RequestedCommandViewAction::ToggleJsonString { path, tree } => {
    match tree {
        McpTree::Request => self.mcp_request_tree_state.toggle_string(path),
        McpTree::Response => self.mcp_response_tree_state.toggle_string(path),
    }
    ctx.notify();
}

Presubmit runs cargo clippy --workspace --all-targets --all-features --tests -- -D warnings, so this will fail CI before Phase 3 merges.

[cephalonaut]

Phase 2 Final Review — APP-2527

Verdict: PASS — all four minors are resolved; no new issues introduced by the API change.

---

✅ M4 — Fixed (API changed to &[PathSegment])

toggle and toggle_string now accept &[PathSegment], allocating with .to_vec() internally only at write time. The action handlers pass the destructured path: Vec<PathSegment> directly via auto-deref — no clone, no redundant allocation:

RequestedCommandViewAction::ToggleJsonNode { path, tree } => {
    let depth = path.len();
    match tree {
        McpTree::Request => self.mcp_request_tree_state.toggle(path, depth),
        McpTree::Response => self.mcp_response_tree_state.toggle(path, depth),
    }
    ctx.notify();
}
RequestedCommandViewAction::ToggleJsonString { path, tree } => {
    match tree {
        McpTree::Request => self.mcp_request_tree_state.toggle_string(path),
        McpTree::Response => self.mcp_response_tree_state.toggle_string(path),
    }
    ctx.notify();
}

All existing test call sites in json_tree_tests.rs were updated from path.clone() to &path. No other call sites for JsonTreeState::toggle / toggle_string exist in the codebase (other .toggle() calls in the repo are on unrelated types). The API change is complete and consistent.

---

All minors resolved

Item	Status
M1 — doc comment (Phase 1, APP-2527)	✅ Fixed
M2 — toggle_string / is_string_expanded tests	✅ Fixed
M3 — separate request/response states, McpTree enum	✅ Fixed
M4 — redundant .clone()	✅ Fixed

Phase 2 is clean. Ready to merge and proceed to Phase 3.