diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 45304bc0f..274f1ba98 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -100,7 +100,7 @@ toml_edit = "0.19" serde_yaml = "0.9" notify = "6" base64 = "0.22" -agent-client-protocol-schema = { version = "0.11", features = ["unstable_session_usage", "unstable_session_fork", "unstable_session_resume"] } +agent-client-protocol-schema = { version = "0.11", features = ["unstable_session_usage", "unstable_session_fork", "unstable_session_resume", "unstable_elicitation"] } kill_tree = { version = "0.2", features = ["tokio"] } which = "7" keyring = { version = "3", features = ["apple-native", "windows-native", "sync-secret-service"], optional = true } diff --git a/src-tauri/src/acp/codex_goal.rs b/src-tauri/src/acp/codex_goal.rs index fd0361a48..4021402f5 100644 --- a/src-tauri/src/acp/codex_goal.rs +++ b/src-tauri/src/acp/codex_goal.rs @@ -248,6 +248,36 @@ mod tests { assert_eq!(input["objective"], "Fix the login bug"); } + #[test] + fn preserves_v114_slimmed_snapshot_fields() { + // codex-acp v1.1.4 (#293) slimmed the goal snapshot: it dropped + // `tokensUsed` and added `createdAt` / `controlMethod` (alongside the + // existing `tokenBudget` / `timeUsedSeconds`). `goal_marker` clones the + // object through, so the new fields survive onto the card output and the + // absent `tokensUsed` is simply not present — `GoalCard` reads it as null + // and hides that stat, so no display breaks. + let goal = json!({ + "objective": " Ship the release ", + "status": "active", + "tokenBudget": 200000, + "timeUsedSeconds": 42, + "createdAt": "2026-07-16T10:00:00Z", + "controlMethod": "_codex/session/goal_control", + }); + let m = goal_marker(&goal).expect("goal marker"); + assert_eq!(m.tool_name, "create_goal"); + let out: Value = serde_json::from_str(&m.output_json).unwrap(); + let g = &out["goal"]; + assert_eq!(g["objective"], "Ship the release"); // trimmed + assert_eq!(g["status"], "active"); + assert_eq!(g["tokenBudget"], 200000); + assert_eq!(g["timeUsedSeconds"], 42); + assert_eq!(g["createdAt"], "2026-07-16T10:00:00Z"); + assert_eq!(g["controlMethod"], "_codex/session/goal_control"); + // Slimmed snapshot carries no tokensUsed → the card hides that stat. + assert!(g.get("tokensUsed").is_none()); + } + #[test] fn goal_tool_call_id_is_occurrence_unique() { // Occurrence-addressed so two runs sharing an objective never collide. diff --git a/src-tauri/src/acp/connection.rs b/src-tauri/src/acp/connection.rs index 5a52fda29..63586871e 100644 --- a/src-tauri/src/acp/connection.rs +++ b/src-tauri/src/acp/connection.rs @@ -4,7 +4,8 @@ use std::sync::Arc; use sacp::schema::{ BlobResourceContents, CancelNotification, ClientCapabilities, ContentBlock, ContentChunk, - CreateTerminalRequest, CreateTerminalResponse, EmbeddedResource, EmbeddedResourceResource, + CreateTerminalRequest, CreateTerminalResponse, + ElicitationCapabilities, ElicitationFormCapabilities, EmbeddedResource, EmbeddedResourceResource, FileSystemCapabilities, ImageContent, InitializeRequest, KillTerminalRequest, KillTerminalResponse, LoadSessionRequest, NewSessionRequest, NewSessionResponse, PermissionOptionKind, Plan, PlanEntryPriority, PlanEntryStatus, PromptRequest, ProtocolVersion, @@ -128,6 +129,23 @@ fn apply_grok_env_policy(merged: &mut Vec<(String, String)>, runtime_env: &BTree } } +/// Codex-only launch policy: force codex-acp's MCP name-conflict de-duplication +/// OFF. codeg injects its companion server (`codeg-mcp`) over ACP +/// `session/new.mcpServers`; codex-acp otherwise drops any ACP-passed server +/// whose name collides with a `config.toml` entry — global *or* project layer +/// (the check was widened to project `.codex/config.toml` in codex-acp #322) — +/// silently stripping codeg-mcp and with it ask_user_question / delegation / +/// feedback / session_info. The late `retain` + `push` makes the override win +/// over any user `runtime_env` twin, so the injection is guaranteed to survive. +fn apply_codex_env_policy(agent_type: AgentType, merged: &mut Vec<(String, String)>) { + if agent_type != AgentType::Codex { + return; + } + let key = "DISABLE_MCP_CONFIG_FILTERING"; + merged.retain(|(k, _)| k != key); + merged.push((key.to_string(), "true".to_string())); +} + /// Prepend `dir` to the PATH entry of `env`, seeding from `fallback_path` when /// `env` has no PATH key of its own. Removes any pre-existing PATH key first /// (case-insensitively when `windows`, since Windows env keys are @@ -192,6 +210,19 @@ fn prepend_officecli_path(env: &mut BTreeMap) { } } +/// The two actions codex's bespoke `_codex/session/goal_control` request +/// accepts (codex-acp #293, v1.1.4). Start / resume / re-objective are NOT part +/// of this method — those go through the `/goal` prompt (a real slash command; +/// only `/plan`, a config-option state toggle, is suppressed). Serializes to the +/// lowercase wire value codex expects (`"pause"` / `"clear"`) and deserializes +/// from the same string coming off the tauri command / HTTP endpoint. +#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum GoalControlAction { + Pause, + Clear, +} + /// Commands sent from Tauri command handlers to the ACP connection loop. pub enum ConnectionCommand { Prompt { @@ -212,6 +243,9 @@ pub enum ConnectionCommand { config_id: String, value_id: String, }, + GoalControl { + action: GoalControlAction, + }, Cancel, RespondPermission { request_id: String, @@ -378,6 +412,7 @@ async fn build_agent( crate::commands::acp::seed_pi_workspace_trust(cwd, runtime_env); } let mut merged_env = merge_agent_env(env, runtime_env); + apply_codex_env_policy(agent_type, &mut merged_env); // codex-acp 1.0.0 honors APP_SERVER_LOGS as a directory for its // adapter-side logs. Surface it only under CODEG_ACP_DEBUG so // default runs are unchanged; a directory-creation failure silently @@ -736,6 +771,23 @@ async fn build_agent( }) } +/// Stack size for the dedicated OS thread that drives each ACP connection's +/// `run_connection` future (see the spawn site below). `run_connection` is one +/// colossal async state machine — the full per-connection message loop plus +/// every registered client-request handler — and in DEBUG builds the compiler +/// does not pack async locals, so a single poll of the connection closure needs +/// far more than a default ~2 MiB Tokio worker-thread stack. Left on the worker +/// pool it overflowed the stack and aborted the whole process under `tauri dev` +/// (release builds pack the frame small enough to fit, which is why only debug +/// crashed). 8 MiB matches the macOS main-thread stack — 4x the default that +/// overflowed, generous headroom for the debug frame as ACP features accrete. +/// The stack is reserved address space, lazily committed, so it costs no +/// physical memory beyond the pages actually touched (~the real 2-3 MiB the +/// frame uses), regardless of this cap — so the larger reservation is free. If a +/// future ACP feature ever grows the loop past even this, split `run_connection` +/// into boxed sub-futures rather than raising it further. +const ACP_CONNECTION_STACK_SIZE: usize = 8 * 1024 * 1024; + /// Spawn an ACP agent process and run the connection loop in a background task. /// /// On success, the newly created `AgentConnection` is inserted into @@ -850,14 +902,30 @@ pub async fn spawn_agent_connection( }, ); - tokio::spawn(async move { - // RAII guard: runs on normal exit AND on panic unwinding, so a - // panic inside `run_connection` can't leak a stale map entry. - let _cleanup = ConnectionCleanupGuard { - connections: cleanup_connections, - connection_id: cleanup_connection_id, - }; - + // Drive `run_connection` on a dedicated, large-stack thread (see + // ACP_CONNECTION_STACK_SIZE) rather than a Tokio worker task: its debug + // poll frame is too big for a default ~2 MiB worker stack and was aborting + // the process under `tauri dev`. `Handle::block_on` runs it on the SAME + // shared runtime, so `tokio::spawn`/timers/IO inside `run_connection` still + // use the pool — only the giant top-level frame moves to the roomy stack. + // The connection is fire-and-forget (torn down from within via `cmd_rx` / + // process exit; no JoinHandle is awaited), so a thread is behaviorally + // equivalent to the previous task. + let connection_rt = tokio::runtime::Handle::current(); + // RAII guard built OUTSIDE the thread body and moved in: on a normal exit + // or panic unwind its Drop removes the manager map entry, AND if the thread + // fails to spawn the dropped closure runs the same Drop — so the entry is + // never leaked. + let cleanup_guard = ConnectionCleanupGuard { + connections: cleanup_connections, + connection_id: cleanup_connection_id, + }; + let connection_thread = std::thread::Builder::new() + .name(format!("acp-conn-{conn_id}")) + .stack_size(ACP_CONNECTION_STACK_SIZE) + .spawn(move || { + let _cleanup = cleanup_guard; + connection_rt.block_on(async move { let delegation_for_cleanup = delegation_injection.clone(); let result = run_connection( agent, @@ -892,6 +960,11 @@ pub async fn spawn_agent_connection( // companion's ask socket to close (which a reparented/hard-killed // agent may never do); the dropped sender declines the tool cleanly. inj.questions.cancel_questions_by_parent(&conn_id).await; + // Likewise reclaim a parked Grok `exit_plan_mode` approval; the + // dropped sender replies disconnect so grok keeps plan mode active. + inj.plan_approvals + .cancel_plan_approvals_by_parent(&conn_id) + .await; } if let Err(e) = result { @@ -934,16 +1007,80 @@ pub async fn spawn_agent_connection( }, ) .await; - // `_cleanup` is dropped here — removes the connection entry from - // the manager map. Same drop semantics apply on panic unwinding. - }); + // Connection loop ended; `block_on` returns and `_cleanup` + // (bound at the top of the thread body) drops next, removing + // the manager map entry — same as on a panic unwind. + }); + }); + if let Err(e) = connection_thread { + // Thread creation only fails on OS resource exhaustion. Dropping the + // un-spawned closure already ran `cleanup_guard`'s Drop (removing the + // map entry), so just surface the failure and let the caller abort. + tracing::error!("[ACP] failed to spawn connection driver thread: {e}"); + return Err(AcpError::SpawnFailed(format!( + "connection driver thread: {e}" + ))); + } Ok(session_started_rx) } +/// A pending permission-card responder. `Acp` is a real ACP +/// `session/request_permission`; `CodexElicitation` is a codex approval-style +/// `elicitation/create` (MCP tool-call approval / message-only confirm) routed +/// through the SAME permission card so approvals look exactly like they did +/// before codeg advertised `elicitation.form` — its chosen option answers the +/// blocked elicitation request instead (see `handle_elicitation_request`). +enum PendingPermission { + Acp(Responder), + CodexElicitation { + responder: Responder, + approval: crate::acp::question::ElicitationApproval, + }, +} + +impl PendingPermission { + /// Resolve with the user's chosen option id. + fn respond_selected(self, option_id: String) { + match self { + PendingPermission::Acp(responder) => { + let outcome = + RequestPermissionOutcome::Selected(SelectedPermissionOutcome::new(option_id)); + let _ = responder.respond(RequestPermissionResponse::new(outcome)); + } + PendingPermission::CodexElicitation { + responder, + approval, + } => { + let response = crate::acp::question::build_elicitation_approval_response( + &approval, &option_id, + ); + let _ = responder.respond(serde_json::to_value(response).unwrap_or_default()); + } + } + } + + /// Resolve as cancelled — the turn ended / connection tore down before the + /// user chose. + fn respond_cancelled(self) { + match self { + PendingPermission::Acp(responder) => { + let _ = responder.respond(RequestPermissionResponse::new( + RequestPermissionOutcome::Cancelled, + )); + } + PendingPermission::CodexElicitation { responder, .. } => { + let _ = responder.respond( + serde_json::to_value(crate::acp::question::elicitation_cancel_response()) + .unwrap_or_default(), + ); + } + } + } +} + /// Shared state for pending permission responders. -type PendingPermissions = - Arc>>>; +type PendingPermissions = Arc>>; fn map_session_modes(mode_state: &SessionModeState) -> SessionModeStateInfo { SessionModeStateInfo { @@ -2015,6 +2152,14 @@ pub struct DelegationInjection { /// the delegation `broker.cancel_by_parent` cleanup. Shares the same backing /// `ConnectionManager` as the listener's question lookup. pub questions: Arc, + /// Plan-approval registry handle for Grok's `exit_plan_mode` ext bridge. + /// Unlike delegation / ask / feedback this is NOT a codeg-mcp feature — it is + /// Grok's native plan mode — so it has no runtime on/off flag and is always + /// wired. Shares the same backing `ConnectionManager` as the question lookup. + /// The `run_connection` handler registers approvals + parks on the reply + /// through this, and the cleanup guard calls `cancel_plan_approvals_by_parent` + /// on disconnect (mirroring the question teardown cascade). + pub plan_approvals: Arc, } /// Locate the `codeg-mcp` companion binary across the supported deployment @@ -2352,6 +2497,14 @@ async fn run_connection( .as_ref() .map(|inj| (Arc::clone(&inj.questions), inj.ask.clone())); let grok_ask_conn_id = connection_id.clone(); + // Grok `exit_plan_mode` bridge access — always wired in production (native + // plan mode, no feature flag). `None` only on the test paths that spin up + // `run_connection` without a delegation stack; the handler then replies + // disconnect and grok keeps plan mode active. + let grok_plan_access = delegation_injection + .as_ref() + .map(|inj| Arc::clone(&inj.plan_approvals)); + let grok_plan_conn_id = connection_id.clone(); // The ext handler emits the answered in-stream card (`AskQuestionResultCard`) // itself once the user submits — grok never emits a completed tool result into // the ACP stream — so it needs this connection's session state + emitter. @@ -2515,18 +2668,77 @@ async fn run_connection( }, on_receive_request!(), ) + .on_receive_request( + { + let access = grok_plan_access.clone(); + let conn_id = grok_plan_conn_id.clone(); + async move |req: GrokExitPlanModeRequest, + responder: Responder, + _cx: ConnectionTo| { + handle_grok_exit_plan_mode(&access, &conn_id, req, responder).await; + Ok(()) + } + }, + on_receive_request!(), + ) + .on_receive_request( + { + // Codex `elicitation/create`: question-style requests (Plan + // mode `request_user_input`, generic MCP forms) bridge into + // the same ask card as the codeg-mcp ask tool (reusing the ask + // access + kill switch); approval-style requests (MCP + // tool-call approvals, message-only confirms) route through + // the permission card via `pending_perms`. + let access = grok_ask_access.clone(); + let conn_id = grok_ask_conn_id.clone(); + let perms = perms.clone(); + let state_inner = Arc::clone(&state); + let emitter_inner = emitter_clone.clone(); + async move |req: CodexElicitationRequest, + responder: Responder, + _cx: ConnectionTo| { + handle_elicitation_request( + &access, + &perms, + &state_inner, + &emitter_inner, + &conn_id, + req, + responder, + ) + .await; + Ok(()) + } + }, + on_receive_request!(), + ) .connect_with(agent, async move |cx| -> Result<(), sacp::Error> { let state = state_outer; let agent_name_for_log = registry::get_agent_meta(agent_type).name; // Advertise filesystem + terminal capabilities for ACP tool execution. - let init_request = InitializeRequest::new(ProtocolVersion::LATEST).client_capabilities( - ClientCapabilities::new() - .terminal(true) - .fs(FileSystemCapabilities::new() - .read_text_file(true) - .write_text_file(true)), - ); + let mut client_capabilities = ClientCapabilities::new() + .terminal(true) + .fs(FileSystemCapabilities::new() + .read_text_file(true) + .write_text_file(true)); + // Codex only: advertise form elicitation so codex's native Plan-mode + // `request_user_input` tool is delivered as an `elicitation/create` + // request (handled below) instead of being silently answered `{}`. + // NOTE this reroutes codex's WHOLE form-elicitation surface — MCP + // tool-call approvals and MCP-server forms included — so the + // handler must cover every shape (`classify_elicitation`). URL + // elicitation is deliberately NOT advertised: codex-acp then falls + // back to `session/request_permission`, which codeg already + // handles. Scoped to Codex to keep the blast radius off other + // agents (e.g. Claude's native AskUserQuestion, which would + // otherwise un-gate and duplicate the codeg-mcp ask tool). + if agent_type == AgentType::Codex { + client_capabilities = client_capabilities + .elicitation(ElicitationCapabilities::new().form(ElicitationFormCapabilities::new())); + } + let init_request = InitializeRequest::new(ProtocolVersion::LATEST) + .client_capabilities(client_capabilities); // Bound the Initialize handshake so an outdated / incompatible // cached binary that never responds can't leave the frontend // stuck on "Connecting...". A healthy agent answers in <1s; we @@ -3190,6 +3402,31 @@ async fn run_connection( #[serde(transparent)] struct GrokAskUserQuestionRequest(serde_json::Value); +/// Store the plan-approval responder and render the approval card. Grok's native +/// `exit_plan_mode` tool issues this ACP ext request (`_x.ai/exit_plan_mode`) and +/// BLOCKS on the reply — the agent won't leave plan mode until the user acts. +/// Transparent over the raw params object (`{sessionId, toolCallId, planContent}`); +/// the fields codeg needs are read by +/// [`crate::acp::plan_approval::parse_grok_exit_plan_request`]. sacp routes typed +/// handlers on the RAW wire method, so the derive keeps the leading `_`. +#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, JsonRpcRequest)] +#[request(method = "_x.ai/exit_plan_mode", response = serde_json::Value)] +#[serde(transparent)] +struct GrokExitPlanModeRequest(serde_json::Value); + +/// Every codex `elicitation/create` request — `request_user_input` (Plan +/// mode), generic MCP-server forms, MCP tool-call approvals, message-only +/// confirms — arrives here once codeg advertises `elicitation.form`. sacp +/// 11.0.0 ships no `JsonRpcRequest`/`JsonRpcResponse` impl for the schema's +/// elicitation types (and no feature to enable them), so — like the grok bridge +/// — take the raw params object and reply with a raw JSON value (the serialized +/// `CreateElicitationResponse`). sacp has no built-in elicitation handling, so +/// this custom method handler fills the gap with no dispatch conflict. +#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, JsonRpcRequest)] +#[request(method = "elicitation/create", response = serde_json::Value)] +#[serde(transparent)] +struct CodexElicitationRequest(serde_json::Value); + /// Bridge grok's native `_x.ai/ask_user_question` ext request into codeg's /// interactive question card. Grok blocks on the reply, so codeg registers the /// questions through the shared [`crate::acp::question::SessionQuestionAccess`] — @@ -3295,6 +3532,294 @@ async fn handle_grok_ask_user_question( }); } +/// Bridge grok's native `_x.ai/exit_plan_mode` ext request into codeg's +/// interactive plan-approval card. Grok BLOCKS on the reply — it won't leave plan +/// mode until the user acts — so codeg registers the approval through the shared +/// [`crate::acp::plan_approval::SessionPlanApprovalAccess`] (which sets +/// `pending_plan_approval`, broadcasts `PlanApprovalRequest`, and renders the card +/// above the composer), then answers the ext request with the user's decision once +/// they submit. Unlike the ask bridge there is no synthesized in-stream card: +/// grok's own `exit_plan_mode` tool_call renders the plan in the transcript (via +/// `PlanModeCard`), mirroring how the permission dialog coexists with the tool +/// call. Every early return replies with the disconnect-shaped response so grok +/// keeps plan mode active — it can never be read as a silent approval. +async fn handle_grok_exit_plan_mode( + access: &Option>, + connection_id: &str, + req: GrokExitPlanModeRequest, + responder: Responder, +) { + // Log the wire SHAPE (top-level field names), not the raw request — the plan + // body can be large and carry file paths / source. The keys are what + // wire-format verification needs (confirm `sessionId`/`toolCallId`/`planContent` + // on the first real run); the malformed path below logs more if parsing fails. + tracing::info!( + "[grok exit_plan] received _x.ai/exit_plan_mode ext request: keys={:?}", + req.0 + .as_object() + .map(|o| o.keys().map(String::as_str).collect::>()) + ); + let Some(access) = access else { + let _ = + responder.respond(crate::acp::plan_approval::grok_exit_plan_disconnect_response()); + return; + }; + let (plan_markdown, tool_call_id) = + match crate::acp::plan_approval::parse_grok_exit_plan_request(&req.0) { + Ok(parsed) => parsed, + Err(e) => { + tracing::warn!("[grok exit_plan] rejecting malformed ext request: {e}"); + let _ = responder + .respond(crate::acp::plan_approval::grok_exit_plan_disconnect_response()); + return; + } + }; + tracing::info!( + "[grok exit_plan] toolCallId={tool_call_id:?} plan_chars={}", + plan_markdown.chars().count() + ); + let Some(registered) = access + .register_plan_approval(connection_id, tool_call_id, plan_markdown) + .await + else { + // Connection gone, or an approval is already pending on this connection. + let _ = + responder.respond(crate::acp::plan_approval::grok_exit_plan_disconnect_response()); + return; + }; + // The user answers out-of-band (the HTTP `answer_plan_approval` endpoint + // resolves the one-shot below), so await it on a task — keeping the ACP + // dispatch loop free — then reply to grok's blocked ext request. The manager's + // `answer_plan_approval` / teardown emit `PlanApprovalResolved` to clear the + // card; this task only unblocks grok. + tokio::spawn(async move { + match registered.answer_rx.await { + Ok(answer) => { + let _ = responder.respond( + crate::acp::plan_approval::build_grok_exit_plan_response(&answer), + ); + } + // Sender dropped: the approval was canceled or the connection tore + // down — reply disconnect so grok keeps plan mode active. + Err(_) => { + let _ = responder + .respond(crate::acp::plan_approval::grok_exit_plan_disconnect_response()); + } + } + }); +} + +/// Bridge codex's `elicitation/create` requests into codeg's interactive +/// surfaces. Codex only sends these when codeg declares `elicitation.form` +/// (see `connect_with`), then BLOCKS on the reply, so every shape must resolve +/// to something the user can act on (see +/// [`crate::acp::question::classify_elicitation`] for the full taxonomy): +/// +/// * Question-style (Plan-mode `request_user_input`, generic MCP forms) → +/// the shared [`crate::acp::question::SessionQuestionAccess`] path — the +/// SAME one the codeg-mcp ask tool and the grok bridge use (it sets +/// `pending_question`, broadcasts `QuestionRequest`, and `AskQuestionCard` +/// renders) — answered once the user submits. +/// * Approval-style (MCP tool-call approvals, message-only confirms) → the +/// permission card via `pending_perms`, exactly like the +/// `session/request_permission` fallback codex-acp used before the +/// capability was advertised. Auto-declining these would reject the tool +/// call (including codeg-mcp's own tools in consent-requiring modes). +/// +/// Question-path early returns DECLINE, which makes codex proceed with its own +/// judgment — no worse than the pre-bridge `{answers:{}}`, so nothing here can +/// regress it. Codex delivers `request_user_input` ONLY as this elicitation and +/// never puts a completed tool_call on the stream, so — like the grok bridge — +/// the question path synthesizes the answered result card itself once the user +/// submits (keyed by the elicitation's tool_call_id). +#[allow(clippy::too_many_arguments)] +async fn handle_elicitation_request( + access: &Option<( + Arc, + crate::acp::question::QuestionRuntimeConfig, + )>, + perms: &PendingPermissions, + state: &Arc>, + emitter: &EventEmitter, + connection_id: &str, + req: CodexElicitationRequest, + responder: Responder, +) { + // The wire reply is the serialized `CreateElicitationResponse` (see the + // newtype above). `Decline` makes codex proceed with its own judgment. + fn decline() -> serde_json::Value { + serde_json::to_value(crate::acp::question::elicitation_decline_response()) + .unwrap_or_default() + } + let raw = req.0; + // Everything codex-acp can send once `elicitation.form` is advertised + // resolves to a plan here — an unhandled shape would silently reject the + // agent's blocked request (an MCP tool-call approval, most damagingly). + let plan = match crate::acp::question::classify_elicitation(&raw) { + Ok(plan) => plan, + Err(e) => { + tracing::warn!("[codex elicitation] declining unrenderable request: {e}"); + let _ = responder.respond(decline()); + return; + } + }; + match plan { + // Approval-style (MCP tool-call approval / message-only confirm): + // render through the permission card — the exact surface these used + // before codeg advertised `elicitation.form` (codex-acp then sent + // `session/request_permission`). Deliberately NOT gated by the + // ask_user_question toggle: this is consent, not an agent question, + // and auto-declining would reject the tool call outright. + crate::acp::question::ElicitationPlan::Approval(approval) => { + let request_id = uuid::Uuid::new_v4().to_string(); + // Mirror codex-acp's own `request_permission` fallback tool_call + // shape (`buildPermissionRequest`) so the frontend permission card + // renders it identically. When codex correlated the approval to an + // already-rendered mcpToolCall item, reuse that id so the card + // attaches to it. + let tool_call = serde_json::json!({ + "toolCallId": approval + .tool_call_id + .clone() + .unwrap_or_else(|| format!("elicitation-{request_id}")), + "title": approval.message, + "kind": "execute", + "status": "pending", + "content": [{ + "type": "content", + "content": {"type": "text", "text": approval.message}, + }], + }); + let options: Vec = approval + .options + .iter() + .map(|o| PermissionOptionInfo { + option_id: o.option_id.clone(), + name: o.label.clone(), + kind: o.kind.to_string(), + }) + .collect(); + perms.lock().await.insert( + request_id.clone(), + PendingPermission::CodexElicitation { + responder, + approval, + }, + ); + emit_with_state( + state, + emitter, + AcpEvent::PermissionRequest { + request_id, + tool_call, + options, + }, + ) + .await; + } + // Question-style (codex `request_user_input`, generic MCP forms): + // bridge into the same ask card as the codeg-mcp ask tool. + crate::acp::question::ElicitationPlan::Questions(questions) => { + let Some((question_access, ask_cfg)) = access else { + let _ = responder.respond(decline()); + return; + }; + // Same kill switch as the codeg-mcp ask tool and the grok bridge: + // when the user has turned ask_user_question off, decline so codex + // proceeds. + if !ask_cfg.is_enabled().await { + let _ = responder.respond(decline()); + return; + } + // register_question consumes the specs; `questions` keeps its copy + // to correlate the answer back to each field when building the + // response. + let Some(registered) = question_access + .register_question(connection_id, questions.specs.clone()) + .await + else { + // Connection gone, or an ask is already pending on this connection. + let _ = responder.respond(decline()); + return; + }; + // Codex advertises an auto-resolution timeout on some + // `request_user_input` asks (`_meta.codex.autoResolutionMs`): + // codex-acp races the elicitation against it and answers + // `{answers: {}}` itself on expiry, ABANDONING this request. Reap + // the by-then-pointless card shortly after so it can't linger as a + // zombie; `cancel_question` is a no-op if the user already + // answered. + if let Some(ms) = crate::acp::question::elicitation_auto_resolution_ms(&raw) { + let reaper_access = Arc::clone(question_access); + let reaper_conn = connection_id.to_string(); + let reaper_qid = registered.question_id.clone(); + tokio::spawn(async move { + tokio::time::sleep(std::time::Duration::from_millis( + ms.saturating_add(2_000), + )) + .await; + reaper_access.cancel_question(&reaper_conn, &reaper_qid).await; + }); + } + // The user answers out-of-band (the `answer_question` endpoint + // resolves the one-shot below), so await it on a task — keeping + // the ACP dispatch loop free — then reply to codex's blocked + // elicitation request. Cloned here so the synthesized card below can + // write through the session state from the spawned task. + let card_state = Arc::clone(state); + let card_emitter = emitter.clone(); + tokio::spawn(async move { + let response = match registered.answer_rx.await { + Ok(outcome) => { + // Surface the answered "提问回答" capsule in-stream — the + // parity codex's `request_user_input` never emits itself: it + // resolves the answer over THIS elicitation round-trip and + // puts no completed tool_call on the ACP stream (so the live + // message has nothing to render otherwise). Emit BEFORE + // unblocking codex so the card lands ahead of its follow-up + // text; codex is blocked on this reply, so nothing races the + // emit. Keyed by the elicitation's tool_call_id so this is + // the single card for the ask and the reloaded history card + // (`codex.rs`, same id) replaces rather than duplicates it. + if let Some(tool_call_id) = questions.tool_call_id.clone() { + emit_with_state( + &card_state, + &card_emitter, + AcpEvent::ToolCall { + tool_call_id, + title: "request_user_input".to_string(), + kind: "other".to_string(), + status: "completed".to_string(), + content: None, + raw_input: Some( + crate::acp::question::grok_result_card_input( + &questions.specs, + ) + .to_string(), + ), + raw_output: Some( + crate::acp::question::grok_result_card_output(&outcome) + .to_string(), + ), + locations: None, + meta: None, + images: None, + }, + ) + .await; + } + crate::acp::question::build_elicitation_response(&questions, &outcome) + } + // Sender dropped: canceled or the connection tore down. + // Decline so codex proceeds with its own judgment. + Err(_) => crate::acp::question::elicitation_decline_response(), + }; + let _ = responder.respond(serde_json::to_value(response).unwrap_or_default()); + }); + } + } +} + async fn handle_permission_request( state: &Arc>, emitter: &EventEmitter, @@ -3353,7 +3878,10 @@ async fn handle_permission_request( } } - perms.lock().await.insert(request_id.clone(), responder); + perms + .lock() + .await + .insert(request_id.clone(), PendingPermission::Acp(responder)); emit_with_state( state, @@ -3445,6 +3973,34 @@ async fn set_session_config_option_inner( Ok(response.config_options) } +/// Send codex's bespoke `_codex/session/goal_control` extension request to pause +/// or clear the session's active goal (codex-acp #293, v1.1.4). Start / resume / +/// re-objective are NOT this method — they go through the `/goal` prompt. +/// +/// codex replies with an empty object and then pushes the resulting goal +/// snapshot as a normal `session_info_update` (`_meta.codex.goal`, or `null` for +/// a clear), which the existing goal-card path renders — so the response value +/// carries nothing to parse and is intentionally discarded. +/// +/// Sent via `UntypedMessage` because `_codex/…` is a codex-private extension +/// method with no sacp typed variant — the same escape hatch used for +/// `session/set_config_option` and `session/fork`. +async fn send_goal_control( + cx: &ConnectionTo, + session_id: &SessionId, + action: GoalControlAction, +) -> Result<(), sacp::Error> { + let params = serde_json::json!({ + "sessionId": session_id, + "action": action, + }); + let untyped_req = UntypedMessage::new("_codex/session/goal_control", params).map_err(|e| { + sacp::util::internal_error(format!("Failed to build goal_control request: {e}")) + })?; + cx.send_request_to(Agent, untyped_req).block_task().await?; + Ok(()) +} + /// Apply user-saved mode and config-option preferences to a freshly-attached /// session BEFORE the initial `session_modes` / `session_config_options` /// events are emitted to the frontend. @@ -4816,11 +5372,8 @@ async fn run_conversation_loop<'a>( request_id, option_id, }) => { - if let Some(responder) = perms.lock().await.remove(&request_id) { - let outcome = RequestPermissionOutcome::Selected( - SelectedPermissionOutcome::new(option_id), - ); - let _ = responder.respond(RequestPermissionResponse::new(outcome)); + if let Some(pending) = perms.lock().await.remove(&request_id) { + pending.respond_selected(option_id); emit_with_state( state, emitter, @@ -4887,6 +5440,22 @@ async fn run_conversation_loop<'a>( .await; } } + Some(ConnectionCommand::GoalControl { action }) => { + if let Err(e) = send_goal_control(&cx, &sid, action).await { + emit_with_state( + state, + emitter, + AcpEvent::Error { + message: format!("Failed to control goal: {e}"), + agent_type: agent_type.to_string(), + code: None, + // Recoverable: a failed pause/clear leaves the turn alive. + terminal: false, + }, + ) + .await; + } + } Some(ConnectionCommand::Cancel) => { // Send CancelNotification to agent to stop the current turn let _ = cx.send_notification_to( @@ -4902,10 +5471,8 @@ async fn run_conversation_loop<'a>( tracked_terminal_tool_calls.clear(); // Also cancel any pending permission requests let mut locked = perms.lock().await; - for (_, responder) in locked.drain() { - let _ = responder.respond(RequestPermissionResponse::new( - RequestPermissionOutcome::Cancelled, - )); + for (_, pending) in locked.drain() { + pending.respond_cancelled(); } drop(locked); // Immediately emit TurnComplete so the frontend @@ -4952,6 +5519,26 @@ async fn run_conversation_loop<'a>( // DelegationCompleted emit. if let Some(inj) = delegation_injection { inj.broker.cancel_by_parent_turn(conn_id).await; + // Reclaim any parked `ask_user_question` / + // Grok `exit_plan_mode` approval owned by this + // connection. Unlike `perms` (drained inline + // above), these registries live on the manager + // and are otherwise only drained on full + // connection teardown -- but a turn-scoped + // Cancel keeps the connection alive. Without + // this the entry lingers, and the + // one-pending-per-connection guard in + // `register_{question,plan_approval}` then + // silently rejects the NEXT one on this + // connection (its card never shows). Idempotent + // with the cleanup-guard drain (empty map -> + // no-op); the dropped sender declines the tool / + // replies disconnect, exactly like the + // permission drain above. + inj.questions.cancel_questions_by_parent(conn_id).await; + inj.plan_approvals + .cancel_plan_approvals_by_parent(conn_id) + .await; } // Drain the prompt response in the background so // the SACP library doesn't log "receiver dropped" @@ -4974,10 +5561,8 @@ async fn run_conversation_loop<'a>( .await; tracked_terminal_tool_calls.clear(); let mut locked = perms.lock().await; - for (_, responder) in locked.drain() { - let _ = responder.respond(RequestPermissionResponse::new( - RequestPermissionOutcome::Cancelled, - )); + for (_, pending) in locked.drain() { + pending.respond_cancelled(); } disconnect_requested = true; break; @@ -5008,11 +5593,8 @@ async fn run_conversation_loop<'a>( request_id, option_id, }) => { - if let Some(responder) = perms.lock().await.remove(&request_id) { - let outcome = RequestPermissionOutcome::Selected( - SelectedPermissionOutcome::new(option_id), - ); - let _ = responder.respond(RequestPermissionResponse::new(outcome)); + if let Some(pending) = perms.lock().await.remove(&request_id) { + pending.respond_selected(option_id); emit_with_state(state, emitter, AcpEvent::PermissionResolved { request_id }) .await; } @@ -5065,6 +5647,25 @@ async fn run_conversation_loop<'a>( .await; } } + Some(ConnectionCommand::GoalControl { action }) => { + let cx = session.connection(); + let sid = session.session_id().clone(); + if let Err(e) = send_goal_control(&cx, &sid, action).await { + emit_with_state( + state, + emitter, + AcpEvent::Error { + message: format!("Failed to control goal: {e}"), + agent_type: agent_type.to_string(), + code: None, + // Recoverable: an idle pause/clear failure leaves the + // connection alive. + terminal: false, + }, + ) + .await; + } + } Some(ConnectionCommand::Cancel) => { let cx = session.connection(); let sid = session.session_id().clone(); @@ -5073,10 +5674,8 @@ async fn run_conversation_loop<'a>( .release_all_for_session(sid.0.as_ref()) .await; let mut locked = perms.lock().await; - for (_, responder) in locked.drain() { - let _ = responder.respond(RequestPermissionResponse::new( - RequestPermissionOutcome::Cancelled, - )); + for (_, pending) in locked.drain() { + pending.respond_cancelled(); } drop(locked); // Cascade-cancel any pending delegations owned by this parent. @@ -5656,6 +6255,84 @@ fn codebuddy_meta_marks_subagent( .is_some_and(|s| !s.is_empty()) } +/// True when a Codex live `tool_call` is a `subAgentActivity` mapping +/// (codex-acp #304, v1.1.3+). codex-acp maps codex `subAgentActivity` +/// notifications onto ACP `tool_call(kind:other)` carrying +/// `_meta.codex.subagent = {threadId, path, activity}`. codeg already renders +/// codex collaboration from the `collabAgentToolCall` path (spawnAgent/wait/ +/// closeAgent — see `collab-tool.ts`) and reconstructs the full nested +/// transcript on history reload from `agent-.jsonl` (see +/// `parsers/codex.rs`), so this new live signal is redundant with what codeg +/// already shows. Suppressed at the emit point (keeping live and DB-reload +/// consistent — a suppressed event is never persisted) to preserve the current +/// live behavior. Gated on Codex. +fn is_codex_subagent_activity( + agent_type: AgentType, + meta: Option<&serde_json::Map>, +) -> bool { + if agent_type != AgentType::Codex { + return false; + } + meta.and_then(|m| m.get("codex")) + .and_then(|codex| codex.get("subagent")) + .is_some() +} + +/// Extract a retryable-turn-error indicator from a Codex `session_info_update`'s +/// `_meta` (codex-acp #289, v1.1.3+). codex ships a transient, auto-retried +/// error as `_meta.codex.error = {message, codexErrorInfo, additionalDetails, +/// turnId, willRetry}` and keeps the prompt alive; it emits this only when +/// `willRetry == true`. Returns `(message, http_status)` when a non-empty +/// message is present. `codexErrorInfo` may be a bare string enum, an object +/// variant carrying an inner `httpStatusCode`, or absent — only the object form +/// yields a status. Defensively refuses a `willRetry == false` payload so a +/// terminal error can never render as "retrying". +fn codex_retry_indicator( + meta: Option<&serde_json::Map>, +) -> Option<(String, Option)> { + let err = meta?.get("codex")?.get("error")?; + if err.get("willRetry").and_then(|v| v.as_bool()) == Some(false) { + return None; + } + let message = err.get("message").and_then(|v| v.as_str())?.trim(); + if message.is_empty() { + return None; + } + let http_status = err + .get("codexErrorInfo") + .and_then(|info| info.as_object()) + .and_then(|obj| obj.values().next()) + .and_then(|inner| inner.get("httpStatusCode")) + .and_then(|v| v.as_i64()); + Some((message.to_string(), http_status)) +} + +/// True when an available command is really a config-option state toggle rather +/// than an invokable slash command (codex-acp #293, v1.1.4). codex advertises +/// e.g. `/plan` as an `AvailableCommand` tagged +/// `_meta.commandAction = {kind:"setConfigOption", configId:"collaboration_mode", +/// value:"plan", resetValue:"default", presentation:"state"}` — codex's signal +/// that the client should represent it as STATE. codeg already surfaces that +/// state as the `collaboration_mode` config-option selector (the generic +/// `SessionConfigOption` path), so also listing `/plan` as a slash command is +/// redundant and its static "Turn plan mode on" description is wrong once plan +/// mode is already on. Suppress these from the command list. Commands with any +/// other action kind (e.g. `/goal`'s `prefixPrompt`, which takes an objective +/// argument) are real commands and kept. Gated on Codex — `commandAction` is a +/// codex-private `_meta` extension (the ACP schema has no such type). +fn is_config_option_state_command( + agent_type: AgentType, + meta: Option<&serde_json::Map>, +) -> bool { + if agent_type != AgentType::Codex { + return false; + } + meta.and_then(|m| m.get("commandAction")) + .and_then(|action| action.get("kind")) + .and_then(|kind| kind.as_str()) + == Some("setConfigOption") +} + /// True when a CodeBuddy sub-agent tool call's `_meta` marks it as a BACKGROUND /// sub-agent (`codebuddy.ai/isBackground == true`). A background sub-agent runs /// concurrently with the main agent, so the suppression-window invariant (parent @@ -6042,6 +6719,13 @@ async fn emit_conversation_update( // Non-text thought chunks are currently ignored. } SessionUpdate::ToolCall(tc) => { + // codex-acp #304 (v1.1.3+) surfaces codex `subAgentActivity` as a + // live `tool_call`; suppress it — it is redundant with the collab + // capsule and the history reconstruction (see + // `is_codex_subagent_activity`). + if is_codex_subagent_activity(agent_type, tc.meta.as_ref()) { + return; + } let tool_call_id = tc.tool_call_id.to_string(); // Grok emits a redundant `tool_call` for its native ask_user_question // alongside the blocking `_x.ai/ask_user_question` ext request codeg @@ -6182,6 +6866,12 @@ async fn emit_conversation_update( .await; } SessionUpdate::ToolCallUpdate(tcu) => { + // Symmetric with the `ToolCall` arm: a follow-up update for a codex + // `subAgentActivity` still carries `_meta.codex.subagent`, so drop + // it too (see `is_codex_subagent_activity`). + if is_codex_subagent_activity(agent_type, tcu.meta.as_ref()) { + return; + } let tool_call_id = tcu.tool_call_id.to_string(); // Suppress the redundant update stream for grok's ask_user_question // (see the ToolCall arm): match the tracked id, or the meta on a late @@ -6368,7 +7058,10 @@ async fn emit_conversation_update( .await; } SessionUpdate::AvailableCommandsUpdate(update) => { - // Some agents (e.g. Claude Code with overlapping user/project slash + // Drop config-option state toggles (codex `/plan` — see + // `is_config_option_state_command`): they're already the + // `collaboration_mode` selector, not invokable commands. Then dedup: + // some agents (e.g. Claude Code with overlapping user/project slash // commands) emit duplicate entries sharing the same name. Keep the // first occurrence so downstream consumers don't render duplicates; // the frontend reducer also dedupes as a defensive measure. @@ -6376,6 +7069,7 @@ async fn emit_conversation_update( let commands: Vec = update .available_commands .iter() + .filter(|cmd| !is_config_option_state_command(agent_type, cmd.meta.as_ref())) .filter(|cmd| seen.insert(cmd.name.clone())) .map(|cmd| { let input_hint = cmd.input.as_ref().map(|input| match input { @@ -6444,6 +7138,21 @@ async fn emit_conversation_update( .await; } } + // codex-acp #289 (v1.1.3+): a retryable turn error rides under + // `_meta.codex.error` (only when `willRetry == true`) and the turn + // stays alive. Surface a transient retry indicator (the frontend + // reuses the Claude API-retry banner); it is NOT a turn failure. + if let Some((message, error_status)) = codex_retry_indicator(info.meta.as_ref()) { + emit_with_state( + state, + emitter, + AcpEvent::TurnRetrying { + message, + error_status, + }, + ) + .await; + } } other => { // Log unhandled update types for debugging @@ -6506,6 +7215,145 @@ mod tests { ToolCallContent::Diff(d) } + /// Clone a `_meta` map out of a JSON object literal, mirroring how codex-acp + /// ships tool-call / session-info `_meta`. + fn meta_map(v: serde_json::Value) -> serde_json::Map { + v.as_object().expect("object").clone() + } + + #[test] + fn codex_subagent_activity_detected_only_for_codex_subagent_meta() { + // codex-acp #304: `_meta.codex.subagent` marks the suppressed activity. + let sub = meta_map(serde_json::json!({ + "codex": { "subagent": { "threadId": "t1", "path": "/root/x", "activity": "started" } } + })); + assert!(is_codex_subagent_activity(AgentType::Codex, Some(&sub))); + // Only Codex is gated — the same meta never suppresses another agent. + assert!(!is_codex_subagent_activity(AgentType::ClaudeCode, Some(&sub))); + // Absent meta and sibling codex meta keys (goal / collaboration) are not + // subagent activity and must render normally. + assert!(!is_codex_subagent_activity(AgentType::Codex, None)); + let goal = meta_map(serde_json::json!({ "codex": { "goal": { "objective": "x" } } })); + assert!(!is_codex_subagent_activity(AgentType::Codex, Some(&goal))); + let collab = meta_map(serde_json::json!({ + "codex": { "collaboration": { "tool": "spawnAgent" } } + })); + assert!(!is_codex_subagent_activity(AgentType::Codex, Some(&collab))); + } + + #[test] + fn config_option_state_command_suppressed_only_for_codex_set_config_action() { + // codex-acp #293: `/plan` is a config-option state toggle (rendered as the + // `collaboration_mode` selector), not an invokable slash command. + let plan = meta_map(serde_json::json!({ + "commandAction": { + "kind": "setConfigOption", + "configId": "collaboration_mode", + "value": "plan", + "resetValue": "default", + "presentation": "state" + } + })); + assert!(is_config_option_state_command(AgentType::Codex, Some(&plan))); + // Gated on Codex — the same meta never suppresses another agent's command. + assert!(!is_config_option_state_command( + AgentType::ClaudeCode, + Some(&plan) + )); + // `/goal` uses a `prefixPrompt` action (takes an objective argument) → a + // real command, kept. + let goal = meta_map(serde_json::json!({ + "commandAction": { "kind": "prefixPrompt", "presentation": "state" } + })); + assert!(!is_config_option_state_command(AgentType::Codex, Some(&goal))); + // Ordinary commands (no `commandAction`) and absent meta are kept. + assert!(!is_config_option_state_command(AgentType::Codex, None)); + let plain = meta_map(serde_json::json!({ "somethingElse": true })); + assert!(!is_config_option_state_command( + AgentType::Codex, + Some(&plain) + )); + } + + #[test] + fn goal_control_action_roundtrips_codex_wire_values() { + // codex-acp #293: `_codex/session/goal_control` expects lowercase + // "pause" / "clear" on the wire, and the same strings arrive from the + // tauri command / HTTP endpoint — both directions must match exactly. + assert_eq!( + serde_json::to_value(GoalControlAction::Pause).unwrap(), + serde_json::json!("pause") + ); + assert_eq!( + serde_json::to_value(GoalControlAction::Clear).unwrap(), + serde_json::json!("clear") + ); + assert_eq!( + serde_json::from_value::(serde_json::json!("pause")).unwrap(), + GoalControlAction::Pause + ); + assert_eq!( + serde_json::from_value::(serde_json::json!("clear")).unwrap(), + GoalControlAction::Clear + ); + } + + #[test] + fn codex_retry_indicator_extracts_message_and_object_http_status() { + // codex-acp #289: object-variant `codexErrorInfo` carries an inner + // `httpStatusCode`; the message + status are surfaced. + let m = meta_map(serde_json::json!({ + "codex": { "error": { + "message": "Reconnecting after provider returned 401", + "codexErrorInfo": { "responseStreamDisconnected": { "httpStatusCode": 401 } }, + "additionalDetails": "HTTP status 401", + "turnId": "turn-id", + "willRetry": true + } } + })); + assert_eq!( + codex_retry_indicator(Some(&m)), + Some(( + "Reconnecting after provider returned 401".to_string(), + Some(401) + )) + ); + } + + #[test] + fn codex_retry_indicator_string_enum_yields_no_status() { + // A bare string `codexErrorInfo` yields the message but no http status. + let m = meta_map(serde_json::json!({ + "codex": { "error": { + "message": "Server overloaded", + "codexErrorInfo": "serverOverloaded", + "willRetry": true + } } + })); + assert_eq!( + codex_retry_indicator(Some(&m)), + Some(("Server overloaded".to_string(), None)) + ); + } + + #[test] + fn codex_retry_indicator_refuses_terminal_empty_and_absent() { + // `willRetry: false` (e.g. 401 auth) must never render a retry banner. + let terminal = meta_map(serde_json::json!({ + "codex": { "error": { "message": "unauthorized", "willRetry": false } } + })); + assert_eq!(codex_retry_indicator(Some(&terminal)), None); + // Blank/whitespace message → nothing to show. + let blank = meta_map(serde_json::json!({ + "codex": { "error": { "message": " ", "willRetry": true } } + })); + assert_eq!(codex_retry_indicator(Some(&blank)), None); + // No `codex.error` at all (a goal-only or empty session_info_update). + let goal_only = meta_map(serde_json::json!({ "codex": { "goal": null } })); + assert_eq!(codex_retry_indicator(Some(&goal_only)), None); + assert_eq!(codex_retry_indicator(None), None); + } + #[test] fn classify_load_failure_resource_not_found_maps_to_code() { assert_eq!( @@ -6643,6 +7491,42 @@ mod tests { } } + #[test] + fn codex_env_policy_forces_mcp_filter_off_and_overrides_user_twin() { + // Codex gets the flag injected so codex-acp never drops the injected + // `codeg-mcp` server on a config.toml name collision. + let mut env = vec![("PATH".to_string(), "/usr/bin".to_string())]; + apply_codex_env_policy(AgentType::Codex, &mut env); + assert!(env + .iter() + .any(|(k, v)| k == "DISABLE_MCP_CONFIG_FILTERING" && v == "true")); + + // A user-supplied twin is replaced (not duplicated) so the override wins. + let mut with_twin = vec![( + "DISABLE_MCP_CONFIG_FILTERING".to_string(), + "false".to_string(), + )]; + apply_codex_env_policy(AgentType::Codex, &mut with_twin); + let hits: Vec<_> = with_twin + .iter() + .filter(|(k, _)| k == "DISABLE_MCP_CONFIG_FILTERING") + .collect(); + assert_eq!(hits.len(), 1, "no duplicate key"); + assert_eq!(hits[0].1, "true", "codeg override wins over user twin"); + } + + #[test] + fn codex_env_policy_is_noop_for_other_agents() { + for agent in [AgentType::Grok, AgentType::ClaudeCode, AgentType::Gemini] { + let mut env = vec![("PATH".to_string(), "/usr/bin".to_string())]; + apply_codex_env_policy(agent, &mut env); + assert!( + !env.iter().any(|(k, _)| k == "DISABLE_MCP_CONFIG_FILTERING"), + "{agent:?} must not receive the codex-only flag" + ); + } + } + #[test] fn synthesize_edit_single_diff_makes_canonical_edit() { let content = vec![diff_content("/a.rs", Some("old line\n"), "new line\n")]; @@ -8688,6 +9572,19 @@ mod tests { async fn cancel_question(&self, _parent_connection_id: &str, _question_id: &str) {} async fn cancel_questions_by_parent(&self, _parent_connection_id: &str) {} } + struct NoPlanApprovals; + #[async_trait::async_trait] + impl crate::acp::plan_approval::SessionPlanApprovalAccess for NoPlanApprovals { + async fn register_plan_approval( + &self, + _parent_connection_id: &str, + _tool_call_id: String, + _plan_markdown: String, + ) -> Option { + None + } + async fn cancel_plan_approvals_by_parent(&self, _parent_connection_id: &str) {} + } let injection = DelegationInjection { broker, tokens: Arc::new(TokenRegistry::default()), @@ -8697,6 +9594,8 @@ mod tests { sessions: crate::acp::session_info::SessionInfoRuntimeConfig::new(), questions: Arc::new(NoQuestions) as Arc, + plan_approvals: Arc::new(NoPlanApprovals) + as Arc, }; let mut servers: Vec = Vec::new(); diff --git a/src-tauri/src/acp/delegation/listener.rs b/src-tauri/src/acp/delegation/listener.rs index 439298018..db57cda62 100644 --- a/src-tauri/src/acp/delegation/listener.rs +++ b/src-tauri/src/acp/delegation/listener.rs @@ -1859,6 +1859,7 @@ mod tests { description: String::new(), }, ], + is_secret: false, }], }) } diff --git a/src-tauri/src/acp/manager.rs b/src-tauri/src/acp/manager.rs index 86e866cd9..17e2c820b 100644 --- a/src-tauri/src/acp/manager.rs +++ b/src-tauri/src/acp/manager.rs @@ -10,12 +10,17 @@ use sea_orm::{ TransactionTrait, }; -use crate::acp::connection::{spawn_agent_connection, AgentConnection, ConnectionCommand}; +use crate::acp::connection::{ + spawn_agent_connection, AgentConnection, ConnectionCommand, GoalControlAction, +}; use crate::acp::error::AcpError; use crate::acp::feedback::{ bounded_feedback_batch, FeedbackItem, FeedbackStatus, PendingFeedback, SessionFeedbackAccess, MAX_FEEDBACK_CHARS, MAX_FEEDBACK_RESPONSE_BYTES, }; +use crate::acp::plan_approval::{ + PlanApprovalAnswer, RegisteredPlanApproval, SessionPlanApprovalAccess, +}; use crate::acp::question::{ build_outcome, QuestionAnswer, QuestionOutcome, QuestionSpec, RegisteredQuestion, SessionQuestionAccess, @@ -210,6 +215,14 @@ pub struct ConnectionManager { /// no cap, no cumulative growth; entries are removed on answer / cancel / /// connection teardown. pending_questions: Arc>>, + /// In-flight Grok `exit_plan_mode` approvals awaiting the user's decision, + /// keyed by the globally-unique `approval_id`. The connection's ext handler + /// parks on the receiver; the answer / cancel path resolves (and removes) the + /// matching sender. Shared across `clone_ref` clones so the connection-facing + /// `register_plan_approval` and the command-facing `answer_plan_approval` + /// touch the same map. At most one per connection (the agent is blocked in + /// its `exit_plan_mode` call) — no cap, no cumulative growth. + pending_plan_approvals: Arc>>, } /// A parked `ask_user_question` awaiting its answer. The `sender` resolves the @@ -221,6 +234,14 @@ struct PendingQuestionEntry { sender: tokio::sync::oneshot::Sender, } +/// A parked Grok `exit_plan_mode` approval awaiting the user's decision. The +/// `sender` resolves the blocked ext-request round-trip in the connection's +/// handler. `parent_connection_id` routes the resolved event + answer. +struct PendingPlanApprovalEntry { + parent_connection_id: String, + sender: tokio::sync::oneshot::Sender, +} + impl Default for ConnectionManager { fn default() -> Self { Self::new() @@ -236,6 +257,7 @@ impl ConnectionManager { delegation_injection: Arc::new(std::sync::OnceLock::new()), probe_locks: Arc::new(Mutex::new(HashMap::new())), pending_questions: Arc::new(Mutex::new(HashMap::new())), + pending_plan_approvals: Arc::new(Mutex::new(HashMap::new())), } } @@ -248,6 +270,7 @@ impl ConnectionManager { delegation_injection: self.delegation_injection.clone(), probe_locks: self.probe_locks.clone(), pending_questions: self.pending_questions.clone(), + pending_plan_approvals: self.pending_plan_approvals.clone(), } } @@ -273,6 +296,7 @@ impl ConnectionManager { delegation_injection: Arc::new(std::sync::OnceLock::new()), probe_locks: Arc::new(Mutex::new(HashMap::new())), pending_questions: Arc::new(Mutex::new(HashMap::new())), + pending_plan_approvals: Arc::new(Mutex::new(HashMap::new())), } } @@ -1157,6 +1181,27 @@ impl ConnectionManager { .map_err(|_| AcpError::ProcessExited) } + /// Pause or clear the session's active Codex goal via the connection loop + /// (codex-acp #293). Looked up by connectionId; the loop sources the + /// sessionId from the live session, so callers only supply the action. + pub async fn goal_control( + &self, + conn_id: &str, + action: GoalControlAction, + ) -> Result<(), AcpError> { + let cmd_tx = { + let connections = self.connections.lock().await; + let conn = connections + .get(conn_id) + .ok_or_else(|| AcpError::ConnectionNotFound(conn_id.into()))?; + conn.cmd_tx.clone() + }; + cmd_tx + .send(ConnectionCommand::GoalControl { action }) + .await + .map_err(|_| AcpError::ProcessExited) + } + pub async fn cancel(&self, db: &DatabaseConnection, conn_id: &str) -> Result<(), AcpError> { let (cmd_tx, state_arc, emitter) = { let connections = self.connections.lock().await; @@ -2220,6 +2265,159 @@ impl ConnectionManager { } } + /// Register a pending Grok `exit_plan_mode` approval on `conn_id`, broadcast + /// `PlanApprovalRequest` (so every attached client renders the card and a + /// mid-turn attach recovers it from the snapshot), and return the receiver + /// the connection's ext handler awaits. `None` when the connection is gone + /// (nothing to approve) OR when one is already pending on it (the agent is + /// blocked in a single `exit_plan_mode` call; a second would orphan the + /// first). Mirrors [`Self::register_question`]. + pub async fn register_plan_approval( + &self, + conn_id: &str, + tool_call_id: String, + plan_markdown: String, + ) -> Option { + let (state, emitter) = self.get_state_and_emitter(conn_id).await?; + let approval_id = uuid::Uuid::new_v4().to_string(); + let (tx, rx) = tokio::sync::oneshot::channel(); + { + let mut reg = self.pending_plan_approvals.lock().await; + if reg.values().any(|e| e.parent_connection_id == conn_id) { + return None; + } + reg.insert( + approval_id.clone(), + PendingPlanApprovalEntry { + parent_connection_id: conn_id.to_string(), + sender: tx, + }, + ); + } + // Ungated emit: the agent is blocked in the tool call, so the card must + // show regardless of any turn-flag timing. + emit_with_state( + &state, + &emitter, + AcpEvent::PlanApprovalRequest { + approval_id: approval_id.clone(), + tool_call_id, + plan_markdown, + }, + ) + .await; + // Teardown event-ordering race (mirrors `register_question`): the + // cleanup guard's `cancel_plan_approvals_by_parent` may have drained this + // entry between the insert above and the emit just now — its + // `PlanApprovalResolved` could then have raced ahead of our + // `PlanApprovalRequest`, leaving a card up with no live backend waiter. + // Emit a compensating `PlanApprovalResolved` (ordered after our request) + // and decline. + if self + .compensate_if_plan_approval_drained(&approval_id, &state, &emitter) + .await + { + return None; + } + Some(RegisteredPlanApproval { + approval_id, + answer_rx: rx, + }) + } + + /// Returns `true` — after emitting a clearing `PlanApprovalResolved` — when + /// `approval_id` is no longer pending, i.e. a teardown sweep drained it in the + /// window after its `PlanApprovalRequest` was broadcast. Mirrors + /// [`Self::compensate_if_question_drained`]. + async fn compensate_if_plan_approval_drained( + &self, + approval_id: &str, + state: &std::sync::Arc>, + emitter: &EventEmitter, + ) -> bool { + if self.pending_plan_approvals.lock().await.contains_key(approval_id) { + return false; + } + emit_with_state( + state, + emitter, + AcpEvent::PlanApprovalResolved { + approval_id: approval_id.to_string(), + }, + ) + .await; + true + } + + /// Resolve a pending plan approval with the user's decision (from any + /// client). Removes the one-shot atomically (first answer wins; a duplicate / + /// already-resolved id is an idempotent no-op), sends the decision to the + /// blocked ext handler, and broadcasts `PlanApprovalResolved` so the card + /// clears on every client. Routing uses the entry's stored parent connection + /// (the `approval_id` is the authoritative key), so a stale `conn_id` from the + /// caller can't misroute. Mirrors [`Self::answer_question`]. + pub async fn answer_plan_approval( + &self, + conn_id: &str, + approval_id: &str, + answer: PlanApprovalAnswer, + ) -> Result<(), AcpError> { + let _ = conn_id; + let entry = self.pending_plan_approvals.lock().await.remove(approval_id); + let Some(entry) = entry else { + // Already answered / canceled / gone elsewhere — idempotent success. + return Ok(()); + }; + // Ignore a dropped receiver: the handler may have abandoned the wait + // (teardown) at the same instant; the resolved event below still clears + // the card. + let _ = entry.sender.send(answer); + if let Some((state, emitter)) = + self.get_state_and_emitter(&entry.parent_connection_id).await + { + emit_with_state( + &state, + &emitter, + AcpEvent::PlanApprovalResolved { + approval_id: approval_id.to_string(), + }, + ) + .await; + } + Ok(()) + } + + /// Cancel every pending plan approval parked on a connection that is tearing + /// down (the `run_connection` cleanup guard calls this). Dropping each + /// entry's sender resolves the handler's await as a disconnect (Grok keeps + /// plan mode active, re-surfaces on reconnect); the `PlanApprovalResolved` + /// broadcast clears the card. Mirrors [`Self::cancel_questions_by_parent`]. + pub async fn cancel_plan_approvals_by_parent(&self, conn_id: &str) { + let drained: Vec = { + let mut reg = self.pending_plan_approvals.lock().await; + let ids: Vec = reg + .iter() + .filter(|(_, e)| e.parent_connection_id == conn_id) + .map(|(id, _)| id.clone()) + .collect(); + for id in &ids { + reg.remove(id); + } + ids + }; + if drained.is_empty() { + return; + } + // Best-effort card clear: the connection may already be out of the map + // (disconnect removes it before this sweep), so tolerate `None`. + if let Some((state, emitter)) = self.get_state_and_emitter(conn_id).await { + for approval_id in drained { + emit_with_state(&state, &emitter, AcpEvent::PlanApprovalResolved { approval_id }) + .await; + } + } + } + /// Resolve a conversation_id to its currently-active connection id, if any. /// Used by the by-conversation snapshot endpoint and the LifecycleSubscriber. /// Per-session state is acquired via `read().await` to avoid the @@ -2538,6 +2736,35 @@ impl SessionQuestionAccess for ConnectionManagerQuestionLookup { } } +/// Production impl of [`SessionPlanApprovalAccess`] for the Grok `exit_plan_mode` +/// ext bridge. Registers / cancels the parent connection's pending plan approval +/// by delegating to `ConnectionManager`. Mirrors `ConnectionManagerQuestionLookup` +/// so the connection handler stays unit-testable with an in-memory stub. +#[derive(Clone)] +pub struct ConnectionManagerPlanApprovalLookup { + pub manager: Arc, +} + +#[async_trait::async_trait] +impl SessionPlanApprovalAccess for ConnectionManagerPlanApprovalLookup { + async fn register_plan_approval( + &self, + parent_connection_id: &str, + tool_call_id: String, + plan_markdown: String, + ) -> Option { + self.manager + .register_plan_approval(parent_connection_id, tool_call_id, plan_markdown) + .await + } + + async fn cancel_plan_approvals_by_parent(&self, parent_connection_id: &str) { + self.manager + .cancel_plan_approvals_by_parent(parent_connection_id) + .await + } +} + #[cfg(test)] mod tests { use super::*; @@ -5391,6 +5618,7 @@ mod tests { description: String::new(), }, ], + is_secret: false, }] } @@ -5509,6 +5737,104 @@ mod tests { assert!(reg_b.answer_rx.await.is_ok()); } + #[tokio::test] + async fn register_then_answer_plan_approval_resolves_and_clears() { + let mgr = ConnectionManager::new(); + mgr.insert_test_connection("pa", AgentType::Grok, None, EventEmitter::Noop) + .await; + let reg = mgr + .register_plan_approval("pa", "call-1".into(), "# Plan\n- step".into()) + .await + .expect("registered"); + // SessionState reflects the pending approval for snapshot recovery. + { + let state = mgr.get_state("pa").await.unwrap(); + let guard = state.read().await; + let pending = guard.pending_plan_approval.as_ref().expect("pending set"); + assert_eq!(pending.plan_markdown, "# Plan\n- step"); + assert_eq!(pending.tool_call_id, "call-1"); + } + + mgr.answer_plan_approval( + "pa", + ®.approval_id, + crate::acp::plan_approval::PlanApprovalAnswer { + decision: crate::acp::plan_approval::PlanApprovalDecision::RequestChanges, + feedback: Some("use SSE".into()), + }, + ) + .await + .unwrap(); + + // The blocked ext handler's receiver resolves with the user's decision. + let got = reg.answer_rx.await.expect("answer delivered"); + assert_eq!( + got.decision, + crate::acp::plan_approval::PlanApprovalDecision::RequestChanges + ); + assert_eq!(got.feedback.as_deref(), Some("use SSE")); + // pending_plan_approval cleared after resolve. + assert!(mgr + .get_state("pa") + .await + .unwrap() + .read() + .await + .pending_plan_approval + .is_none()); + + // Idempotent: answering an already-resolved id is a no-op success. + mgr.answer_plan_approval( + "pa", + ®.approval_id, + crate::acp::plan_approval::PlanApprovalAnswer { + decision: crate::acp::plan_approval::PlanApprovalDecision::Approve, + feedback: None, + }, + ) + .await + .unwrap(); + } + + #[tokio::test] + async fn register_plan_approval_refuses_second_pending_on_same_connection() { + // At most one approval per connection (the agent is blocked in exit_plan_mode). + let mgr = ConnectionManager::new(); + mgr.insert_test_connection("pa2", AgentType::Grok, None, EventEmitter::Noop) + .await; + let _reg = mgr + .register_plan_approval("pa2", "c1".into(), "plan".into()) + .await + .expect("first registers"); + assert!(mgr + .register_plan_approval("pa2", "c2".into(), "plan2".into()) + .await + .is_none()); + } + + #[tokio::test] + async fn cancel_plan_approvals_by_parent_drops_sender_and_clears() { + let mgr = ConnectionManager::new(); + mgr.insert_test_connection("pax", AgentType::Grok, None, EventEmitter::Noop) + .await; + let reg = mgr + .register_plan_approval("pax", "c".into(), "plan".into()) + .await + .unwrap(); + mgr.cancel_plan_approvals_by_parent("pax").await; + // Dropping the sender surfaces to the parked handler as a recv error + // (which it renders as a disconnect reply — plan mode stays active). + assert!(reg.answer_rx.await.is_err()); + assert!(mgr + .get_state("pax") + .await + .unwrap() + .read() + .await + .pending_plan_approval + .is_none()); + } + #[tokio::test] async fn compensate_clears_card_when_entry_drained_before_request_emit() { // Regression for the teardown event-ordering race: register inserts, the diff --git a/src-tauri/src/acp/mod.rs b/src-tauri/src/acp/mod.rs index e2565f193..25007023b 100644 --- a/src-tauri/src/acp/mod.rs +++ b/src-tauri/src/acp/mod.rs @@ -16,6 +16,7 @@ pub mod lifecycle; pub mod manager; pub mod opencode_catalog; pub mod opencode_plugins; +pub mod plan_approval; pub mod preflight; pub mod question; pub mod registry; diff --git a/src-tauri/src/acp/plan_approval.rs b/src-tauri/src/acp/plan_approval.rs new file mode 100644 index 000000000..87726e1a7 --- /dev/null +++ b/src-tauri/src/acp/plan_approval.rs @@ -0,0 +1,306 @@ +//! Grok plan-mode approval ("exit plan mode") domain types. +//! +//! When a Grok session is in **plan mode** and the agent finishes planning, it +//! calls its native `exit_plan_mode` tool. That tool reads `plan.md` from disk +//! and issues a BLOCKING ACP ext request (`_x.ai/exit_plan_mode`, +//! `ExitPlanModeExtRequest { sessionId, toolCallId, planContent }`) to the client +//! to get the user's approval before leaving plan mode. Grok waits for the reply. +//! +//! Codeg bridges that ext request into an interactive **plan-approval card** +//! rendered above the composer — the SAME shape as the `ask_user_question` bridge +//! ([`crate::acp::question`]) and the permission dialog: a pending request is +//! captured onto [`crate::acp::session_state::SessionState`] (in-memory, turn +//! scoped, carried on `to_snapshot()` so a mid-turn attach re-renders the card), +//! and the blocked ext request is answered once the user picks an action. +//! +//! Three outcomes exist (per Grok's binary + `~/.grok/docs/user-guide/19-plan-mode.md`): +//! * **Approve** — leave plan mode and start implementing. +//! * **Request changes** — freeform revision notes; the agent revises and plan +//! mode stays active (the user can iterate). +//! * **Abandon** — abandon the plan and turn plan mode off. +//! +//! ## Wire-format note +//! +//! The request shape (`planContent` / `toolCallId`) is high-confidence. The +//! reply `ExitPlanModeExtResponse` is a 2-field struct (a decision + optional +//! feedback) whose exact serde encoding is INFERRED from binary analysis, not yet +//! captured from a live run (the same state `ask_user_question` was in before it +//! was confirmed against Grok 0.2.101). [`build_grok_exit_plan_response`] is the +//! single isolated function to correct once the first real run confirms the shape; +//! a wrong reply only fails Grok's tool (plan mode stays active, reappears on +//! reconnect) — strictly better and recoverable versus the current silent stall. + +use async_trait::async_trait; +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; +use serde_json::Value; +use tokio::sync::oneshot; + +/// Per-field sanity bound (characters) for the plan markdown carried in the +/// request event + snapshot. The plan rides the broadcast `PlanApprovalRequest` +/// and every snapshot, so this caps the blast radius of a pathological payload. +/// Generous — a real `plan.md` is far smaller. +pub const MAX_PLAN_MARKDOWN_CHARS: usize = 262_144; +/// Per-field sanity bound (characters) for the user's freeform "request changes" +/// feedback, echoed back to Grok. Generous enough for real revision notes. +pub const MAX_FEEDBACK_CHARS: usize = 16_384; + +/// The pending (awaiting-decision) plan approval stored on +/// `SessionState.pending_plan_approval` and carried on `to_snapshot()` so a +/// client attaching mid-turn (cold attach, reconnect, another window) re-renders +/// the approval card even though the one-shot `PlanApprovalRequest` event won't +/// replay for it. At most one is pending per connection (the agent is blocked in +/// its `exit_plan_mode` tool call). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PendingPlanApprovalState { + /// Backend-minted correlation key for the answer (NOT Grok's toolCallId). + pub approval_id: String, + /// Grok's `toolCallId` for the `exit_plan_mode` call, so the synthesized + /// in-stream result card can key on the same id as the (suppressed) native + /// tool call. Empty when the request omitted it. + pub tool_call_id: String, + /// The plan markdown read from `plan.md` (Grok's `planContent`). May be empty + /// — an empty/missing plan still opens the approval surface with an + /// empty-state notice (per Grok's plan-mode doc). + pub plan_markdown: String, + pub created_at: DateTime, +} + +/// Which action the user took on the plan-approval card. `snake_case` on the +/// wire (`approve` / `request_changes` / `abandon`) — constructed by the frontend. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum PlanApprovalDecision { + /// Approve the plan → Grok leaves plan mode and starts implementing. + Approve, + /// Request changes → Grok revises the plan; plan mode stays active. + RequestChanges, + /// Abandon the plan → plan mode is turned off. + Abandon, +} + +/// The user's submission for a pending plan approval (frontend → backend → the +/// blocked ext request). `feedback` carries the freeform revision notes for +/// `RequestChanges` (ignored / empty for the other decisions). camelCase on the +/// wire — this is constructed by the frontend, not read from an event payload. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PlanApprovalAnswer { + pub decision: PlanApprovalDecision, + #[serde(default)] + pub feedback: Option, +} + +impl PlanApprovalAnswer { + /// The trimmed, length-bounded feedback text (empty when none / whitespace). + /// Applied on the answer side so a hand-rolled client hitting the plain + /// `acp_answer_plan_approval` endpoint directly can't ride an unbounded blob + /// back to Grok. + pub fn normalized_feedback(&self) -> String { + self.feedback + .as_deref() + .unwrap_or("") + .trim() + .chars() + .take(MAX_FEEDBACK_CHARS) + .collect() + } +} + +/// What [`SessionPlanApprovalAccess::register_plan_approval`] hands back to the +/// connection's `exit_plan_mode` ext handler: the new approval id plus the +/// receiver to await the user's decision on. Mirrors +/// [`crate::acp::question::RegisteredQuestion`]. +pub struct RegisteredPlanApproval { + pub approval_id: String, + pub answer_rx: oneshot::Receiver, +} + +/// Connection-facing access to register / cancel a pending plan approval on a +/// parent connection. The production impl +/// (`crate::acp::manager::ConnectionManagerPlanApprovalLookup`) wraps the +/// `ConnectionManager`; tests use an in-memory stub. Mirrors +/// [`crate::acp::question::SessionQuestionAccess`] — kept in this neutral module +/// so `connection.rs` depends only on the trait, not on `manager.rs`. +#[async_trait] +pub trait SessionPlanApprovalAccess: Send + Sync { + /// Register a pending plan approval on the parent connection (resolved from + /// the connection id), broadcast `PlanApprovalRequest` to every attached + /// client, and return a receiver that resolves when the user picks an action + /// (or the approval is canceled). `None` when the connection is gone or one + /// is already pending on it — the ext handler then declines to Grok, which + /// keeps plan mode active. + async fn register_plan_approval( + &self, + parent_connection_id: &str, + tool_call_id: String, + plan_markdown: String, + ) -> Option; + + /// Cancel every pending plan approval parked on a connection that is tearing + /// down. Called from the `run_connection` cleanup guard so a pending approval + /// — and the ext handler task parked on it — is reclaimed synchronously on + /// disconnect. Dropping the sender resolves the handler's await as a + /// disconnect (Grok keeps plan mode active, re-surfaces on reconnect). + async fn cancel_plan_approvals_by_parent(&self, parent_connection_id: &str); +} + +/// Parse Grok's `_x.ai/exit_plan_mode` ext-request params into the plan markdown +/// and `toolCallId` codeg needs to render the approval card. Grok's wire shape is +/// `{ sessionId, toolCallId, planContent }`. Lenient: an empty / missing +/// `planContent` is valid (the empty-plan approval surface), so the only hard +/// error is a non-object payload. The markdown is length-bounded to +/// [`MAX_PLAN_MARKDOWN_CHARS`]. +pub fn parse_grok_exit_plan_request(params: &Value) -> Result<(String, String), String> { + let obj = params + .as_object() + .ok_or_else(|| "exit_plan_mode ext request params is not an object".to_string())?; + let plan_markdown: String = obj + .get("planContent") + .and_then(|v| v.as_str()) + .unwrap_or("") + .chars() + .take(MAX_PLAN_MARKDOWN_CHARS) + .collect(); + let tool_call_id = obj + .get("toolCallId") + .and_then(|v| v.as_str()) + .unwrap_or("") + .to_string(); + Ok((plan_markdown, tool_call_id)) +} + +/// Serialize the user's decision into Grok's `ExitPlanModeExtResponse` — the +/// reply to a blocked `_x.ai/exit_plan_mode` ext request. +/// +/// **Verify-on-real-run function.** The reply is a 2-field struct; the exact +/// serde field names/variant strings are inferred (`{ decision, feedback }` with +/// `approve` / `request_changes` / `abandon`) and should be confirmed against the +/// `[grok exit_plan]` request log on the first real run, then corrected HERE if +/// Grok rejects the shape. Everything else in the pipeline is shape-agnostic, so +/// this is the only place to touch. A wrong reply fails Grok's tool (plan mode +/// stays active) rather than corrupting state. +pub fn build_grok_exit_plan_response(answer: &PlanApprovalAnswer) -> Value { + let decision = match answer.decision { + PlanApprovalDecision::Approve => "approve", + PlanApprovalDecision::RequestChanges => "request_changes", + PlanApprovalDecision::Abandon => "abandon", + }; + serde_json::json!({ + "decision": decision, + "feedback": answer.normalized_feedback(), + }) +} + +/// The `_x.ai/exit_plan_mode` reply for a connection tearing down mid-approval +/// (the parked responder is drained without a user decision). Mirrors Grok's own +/// "client disconnected mid-approval; plan mode stays active" behavior: report an +/// error-shaped reply so Grok keeps plan mode active and re-surfaces the approval +/// on reconnect, rather than silently proceeding as if approved. +pub fn grok_exit_plan_disconnect_response() -> Value { + serde_json::json!({ "decision": "cancelled", "feedback": "" }) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn grok_params(plan: &str) -> Value { + json!({ + "sessionId": "s-1", + "toolCallId": "call-42", + "planContent": plan, + }) + } + + #[test] + fn parse_reads_plan_and_tool_call_id() { + let (plan, tc) = parse_grok_exit_plan_request(&grok_params("# Plan\n- step")).unwrap(); + assert_eq!(plan, "# Plan\n- step"); + assert_eq!(tc, "call-42"); + } + + #[test] + fn parse_accepts_empty_or_missing_plan() { + let (plan, tc) = parse_grok_exit_plan_request(&grok_params("")).unwrap(); + assert_eq!(plan, ""); + assert_eq!(tc, "call-42"); + // Missing planContent + toolCallId → empty strings, still Ok. + let (plan, tc) = + parse_grok_exit_plan_request(&json!({ "sessionId": "s-1" })).unwrap(); + assert!(plan.is_empty()); + assert!(tc.is_empty()); + } + + #[test] + fn parse_rejects_non_object() { + assert!(parse_grok_exit_plan_request(&json!("nope")).is_err()); + assert!(parse_grok_exit_plan_request(&json!([1, 2, 3])).is_err()); + } + + #[test] + fn parse_bounds_plan_markdown() { + let huge = "x".repeat(MAX_PLAN_MARKDOWN_CHARS + 500); + let (plan, _) = parse_grok_exit_plan_request(&grok_params(&huge)).unwrap(); + assert_eq!(plan.chars().count(), MAX_PLAN_MARKDOWN_CHARS); + } + + #[test] + fn build_response_maps_each_decision() { + let approve = PlanApprovalAnswer { + decision: PlanApprovalDecision::Approve, + feedback: None, + }; + assert_eq!(build_grok_exit_plan_response(&approve)["decision"], "approve"); + assert_eq!(build_grok_exit_plan_response(&approve)["feedback"], ""); + + let changes = PlanApprovalAnswer { + decision: PlanApprovalDecision::RequestChanges, + feedback: Some(" use SSE instead ".into()), + }; + let v = build_grok_exit_plan_response(&changes); + assert_eq!(v["decision"], "request_changes"); + // Feedback is trimmed. + assert_eq!(v["feedback"], "use SSE instead"); + + let abandon = PlanApprovalAnswer { + decision: PlanApprovalDecision::Abandon, + feedback: None, + }; + assert_eq!(build_grok_exit_plan_response(&abandon)["decision"], "abandon"); + } + + #[test] + fn feedback_is_bounded() { + let huge = "y".repeat(MAX_FEEDBACK_CHARS + 100); + let answer = PlanApprovalAnswer { + decision: PlanApprovalDecision::RequestChanges, + feedback: Some(huge), + }; + assert_eq!( + answer.normalized_feedback().chars().count(), + MAX_FEEDBACK_CHARS + ); + } + + #[test] + fn answer_deserializes_from_camel_case_wire() { + let a: PlanApprovalAnswer = + serde_json::from_value(json!({ "decision": "request_changes", "feedback": "x" })) + .unwrap(); + assert_eq!(a.decision, PlanApprovalDecision::RequestChanges); + assert_eq!(a.feedback.as_deref(), Some("x")); + // feedback optional. + let a: PlanApprovalAnswer = + serde_json::from_value(json!({ "decision": "approve" })).unwrap(); + assert_eq!(a.decision, PlanApprovalDecision::Approve); + assert!(a.feedback.is_none()); + } + + #[test] + fn disconnect_response_is_not_an_approval() { + let v = grok_exit_plan_disconnect_response(); + assert_ne!(v["decision"], "approve"); + } +} diff --git a/src-tauri/src/acp/question.rs b/src-tauri/src/acp/question.rs index 6a1c257dd..9963b6501 100644 --- a/src-tauri/src/acp/question.rs +++ b/src-tauri/src/acp/question.rs @@ -29,8 +29,14 @@ use std::sync::Arc; use async_trait::async_trait; use chrono::{DateTime, Utc}; +use sacp::schema::{ + CreateElicitationRequest, CreateElicitationResponse, ElicitationAcceptAction, ElicitationAction, + ElicitationContentValue, ElicitationMode, ElicitationPropertySchema, ElicitationScope, + MultiSelectItems, StringPropertySchema, +}; use serde::{Deserialize, Serialize}; use serde_json::Value; +use std::collections::BTreeMap; use tokio::sync::{oneshot, RwLock}; /// Max questions per `ask_user_question` call. Matches Claude Code's @@ -74,8 +80,17 @@ pub struct QuestionSpec { pub header: String, /// When true the user may select multiple options. pub multi_select: bool, - /// The choices ([`MIN_OPTIONS`]..=[`MAX_OPTIONS`]). + /// The choices (0..=[`MAX_OPTIONS`]). Empty means free-text: the card + /// renders only its always-present "Other" input (codex `request_user_input` + /// and MCP-server elicitations both ask open questions this way; the + /// codeg-mcp ask tool still requires [`MIN_OPTIONS`] at its own parse + /// layer). pub options: Vec, + /// True when the answer is a secret (codex `request_user_input` marks API + /// keys etc. with `_meta.codex.isSecret`): the card masks the free-text + /// input. Default false — absent on the wire for every non-secret source. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub is_secret: bool, } /// The pending (awaiting-answer) question set stored on @@ -276,6 +291,7 @@ pub fn parse_questions(arguments: &Value) -> Result, String> { header: header.to_string(), multi_select, options, + is_secret: false, }); } Ok(out) @@ -323,9 +339,14 @@ pub fn validate_specs(specs: &[QuestionSpec]) -> Result<(), String> { "questions[{qi}] `header` exceeds {MAX_HEADER_CHARS} characters" )); } - if q.options.len() < MIN_OPTIONS || q.options.len() > MAX_OPTIONS { + // Unlike `parse_questions` (the codeg-mcp tool contract, which keeps + // its [`MIN_OPTIONS`] floor), typed specs allow 0..=[`MAX_OPTIONS`]: + // a question with no options is a legal free-text ask — the card + // renders its always-present "Other" input alone (codex elicitation + // and MCP-server forms ask open questions this way). + if q.options.len() > MAX_OPTIONS { return Err(format!( - "questions[{qi}] must have between {MIN_OPTIONS} and {MAX_OPTIONS} options" + "questions[{qi}] must have at most {MAX_OPTIONS} options" )); } let mut seen_labels = std::collections::HashSet::new(); @@ -526,6 +547,7 @@ pub fn parse_grok_ext_questions(params: &Value) -> Result, Str header: synthesize_grok_header(question), multi_select, options, + is_secret: false, }); } Ok(out) @@ -573,19 +595,621 @@ pub fn grok_ext_skip_response() -> Value { serde_json::json!({ "outcome": "skip_interview" }) } +/// One selectable choice lifted from a schema property: the display `label` +/// (`title`, falling back to the wire value) and the `value` (`const` / `enum` +/// entry) the accept response must send back. The two differ when a generic +/// MCP server elicits with `enum` + `enumNames` (codex-acp normalizes that to +/// `oneOf` with `const` ≠ `title`); codex's own `request_user_input` sets them +/// identical. The typed schema drops per-option descriptions, so they are left +/// empty (the card renders fine without one). +struct ElicitationChoice { + label: String, + value: String, +} + +fn string_prop_choices(s: &StringPropertySchema) -> Vec { + if let Some(one_of) = &s.one_of { + return one_of + .iter() + .map(|o| ElicitationChoice { + label: if o.title.trim().is_empty() { + o.value.clone() + } else { + o.title.clone() + }, + value: o.value.clone(), + }) + .collect(); + } + if let Some(values) = &s.enum_values { + return values + .iter() + .map(|v| ElicitationChoice { + label: v.clone(), + value: v.clone(), + }) + .collect(); + } + Vec::new() +} + +fn multi_select_choices(items: &MultiSelectItems) -> Vec { + match items { + MultiSelectItems::Titled(t) => t + .options + .iter() + .map(|o| ElicitationChoice { + label: if o.title.trim().is_empty() { + o.value.clone() + } else { + o.title.clone() + }, + value: o.value.clone(), + }) + .collect(), + MultiSelectItems::Untitled(u) => u + .values + .iter() + .map(|v| ElicitationChoice { + label: v.clone(), + value: v.clone(), + }) + .collect(), + // `MultiSelectItems` is `#[non_exhaustive]`; an unknown item kind + // yields no options — the question degrades to free text. + _ => Vec::new(), + } +} + +/// How one form field's answer must be typed in the elicitation response. +/// MCP servers validate the accepted content against their schema, so a +/// boolean/number field answered with a string would be rejected — each +/// answer is rebuilt into the schema's own primitive. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ElicitationFieldKind { + /// Single-select or free-text string → bare string. + Text, + /// Multi-select array → string array. + MultiSelect, + /// Yes/No choice → JSON boolean. + Boolean, + /// Free-text parsed as f64 → JSON number (unparseable answers are omitted). + Number, + /// Free-text parsed as i64 → JSON integer (unparseable answers are omitted). + Integer, +} + +/// The response-side rebuild plan for one schema property, parallel (same +/// index) to the [`QuestionSpec`] minted for it. Keeps the property id, the +/// primitive kind, and the label→value map for choice fields — everything +/// [`build_elicitation_response`] needs that [`QuestionSpec`] (a shared wire +/// type the card renders) must not carry. +pub struct ElicitationField { + /// Schema property name — the key the accepted content is sent under. + pub id: String, + pub kind: ElicitationFieldKind, + /// Display label → wire value for choice fields. A label the user typed + /// via "Other" (absent here) is sent verbatim. + pub value_by_label: std::collections::HashMap, +} + +/// A form elicitation that renders as ask-card questions. +pub struct ElicitationQuestions { + pub specs: Vec, + /// Parallel to `specs` (same index). + pub fields: Vec, + /// The elicitation's `toolCallId` (codex sets it to the `request_user_input` + /// item id). The connection handler keys the synthesized in-stream result + /// card to it, so the live card and the reloaded history card (parsed by + /// `codex.rs` from the same id) are one card. `None` when the wire carried + /// no session scope tool_call_id. + pub tool_call_id: Option, +} + +/// One selectable option of an approval-style elicitation, shaped for the +/// permission card (`PermissionOptionInfo` mirrors these fields 1:1). +pub struct ElicitationApprovalOption { + pub option_id: String, + pub label: String, + /// `allow_once` / `allow_always` / `reject_once` — the permission-option + /// kinds the frontend already styles. + pub kind: &'static str, +} + +/// The reserved option id for rejecting an approval-style elicitation. Never +/// collides with a persist `const` (codex only mints `once`/`session`/`always`) +/// and is filtered out of the accept mapping defensively. +pub const ELICITATION_DECLINE_OPTION_ID: &str = "__decline"; + +/// A form elicitation that renders through the PERMISSION card rather than the +/// ask card: an MCP tool-call approval (`_meta.codex_approval_kind == +/// "mcp_tool_call"`) or a message-only confirm (a form with no renderable +/// fields). Routing these through the permission flow keeps MCP approvals +/// looking exactly like they did before codeg advertised `elicitation.form` +/// (codex-acp then used `session/request_permission` with the same options). +pub struct ElicitationApproval { + /// The human prompt ("Allow tool call?" / the MCP server's message). + pub message: String, + /// The tool call this approval correlates to (codex emits the mcpToolCall + /// item before eliciting), when the wire carried one. + pub tool_call_id: Option, + /// The selectable options in display order; always ends with the + /// [`ELICITATION_DECLINE_OPTION_ID`] entry. + pub options: Vec, + /// True when the accept response must echo the chosen option back as + /// `content.persist` (the request carried codex's persist-choice + /// property; codex-acp pops it into the app-server `_meta.persist`). + pub persist_in_content: bool, +} + +/// How a form `elicitation/create` request must be presented. +pub enum ElicitationPlan { + Questions(ElicitationQuestions), + Approval(ElicitationApproval), +} + +/// Codex's synthetic free-text "Other" companion fields are named +/// `__other` (`__other1`, `__other2`… on collision). The card +/// offers its own "Other" on every question, so companions are skipped and the +/// typed answer rides the main field (codex falls back to it). +fn is_other_companion(id: &str) -> bool { + let Some(pos) = id.rfind("__other") else { + return false; + }; + pos > 0 && id[pos + "__other".len()..].chars().all(|c| c.is_ascii_digit()) +} + +/// True when the request is codex's MCP tool-call approval elicitation. The +/// marker rides the request `_meta` verbatim (codex-acp passes the app-server +/// `_meta` through), NOT under the `codex` namespace. +fn is_mcp_tool_call_approval(raw: &Value) -> bool { + raw.get("_meta") + .and_then(|m| m.get("codex_approval_kind")) + .and_then(Value::as_str) + == Some("mcp_tool_call") +} + +/// Codex's auto-resolution timeout for a `request_user_input` elicitation +/// (`_meta.codex.autoResolutionMs`). When set, codex-acp races the elicitation +/// against this timer and answers `{answers: {}}` itself on expiry — the +/// connection handler mirrors it to reap the by-then-pointless card. +pub fn elicitation_auto_resolution_ms(raw: &Value) -> Option { + raw.get("_meta")? + .get("codex")? + .get("autoResolutionMs")? + .as_u64() +} + +/// Classify a form `elicitation/create` request (the raw JSON params) into its +/// presentation plan. Everything codex-acp can send once `elicitation.form` is +/// advertised lands here, so every shape must resolve to SOMETHING the user +/// can act on — an unhandled shape would stall or silently reject the agent's +/// blocked request: +/// +/// * MCP tool-call approvals (`_meta.codex_approval_kind`) → [`ElicitationApproval`] +/// with the persist choices (`once`/`session`/`always`) + Decline — the +/// permission card, exactly like the pre-capability `request_permission` +/// fallback. This includes approvals for codeg-mcp's OWN tools in +/// consent-requiring permission modes, so declining them here would break +/// ask/delegation for codex outright. +/// * Forms with no renderable fields (message-only MCP confirms) → +/// [`ElicitationApproval`] with Accept/Decline. +/// * Everything else (codex `request_user_input`, generic MCP forms) → +/// [`ElicitationQuestions`]: string `oneOf`/`enum` render as choices +/// (title displayed, `const` sent back), plain strings/numbers/integers as +/// free text, booleans as Yes/No, arrays as multi-select; `__other` +/// companions are skipped (the card has its own "Other"). +/// +/// Counts are clamped to codeg's bounds because +/// [`crate::acp::manager::ConnectionManager::register_question`] re-runs +/// [`validate_specs`]. Errors only on non-form / undeserializable requests, +/// which the connection handler turns into a graceful decline. +pub fn classify_elicitation(raw: &Value) -> Result { + let req: CreateElicitationRequest = serde_json::from_value(raw.clone()) + .map_err(|e| format!("unparseable elicitation request: {e}"))?; + let ElicitationMode::Form(form) = &req.mode else { + // codeg only advertises `elicitation.form` — URL mode goes down + // codex-acp's `request_permission` fallback and never reaches here. + return Err("elicitation is not form mode".to_string()); + }; + let message: String = req + .message + .trim() + .chars() + .take(MAX_QUESTION_TEXT_CHARS) + .collect(); + let tool_call_id = match &form.scope { + ElicitationScope::Session(s) => s.tool_call_id.as_ref().map(|t| t.0.to_string()), + _ => None, + }; + if is_mcp_tool_call_approval(raw) { + return Ok(ElicitationPlan::Approval(approval_from_form( + form, + message, + tool_call_id, + ))); + } + let mut questions = parse_form_questions(form, raw); + if questions.specs.is_empty() { + // A form with nothing to fill in is a bare confirmation — mirror + // codex-acp's own no-capability fallback (Accept/Decline options). + return Ok(ElicitationPlan::Approval(ElicitationApproval { + message, + tool_call_id, + options: vec![ + ElicitationApprovalOption { + option_id: "accept".to_string(), + label: "Accept".to_string(), + kind: "allow_once", + }, + decline_approval_option(), + ], + persist_in_content: false, + })); + } + // Carry the elicitation's tool_call_id so the connection handler can key the + // synthesized in-stream result card to it — codex delivers `request_user_input` + // ONLY as this elicitation, never as a stream tool_call. + questions.tool_call_id = tool_call_id; + Ok(ElicitationPlan::Questions(questions)) +} + +fn decline_approval_option() -> ElicitationApprovalOption { + ElicitationApprovalOption { + option_id: ELICITATION_DECLINE_OPTION_ID.to_string(), + label: "Decline".to_string(), + kind: "reject_once", + } +} + +/// Build the approval plan for an MCP tool-call approval elicitation. The +/// request's only (optional) field is codex-acp's injected `persist` choice — +/// `oneOf` of `once` (+ `session`/`always` when codex advertises them). Its +/// options become the allow choices (wire titles displayed, `const` as the +/// option id) plus Decline; with no persist property the choices are plain +/// Allow/Decline. Mirrors codex-acp's own `request_permission` fallback +/// (`buildToolApprovalOptions`) so approvals look identical either way. +fn approval_from_form( + form: &sacp::schema::ElicitationFormMode, + message: String, + tool_call_id: Option, +) -> ElicitationApproval { + let persist_choices = form + .requested_schema + .properties + .get("persist") + .and_then(|p| match p { + ElicitationPropertySchema::String(s) => Some(string_prop_choices(s)), + _ => None, + }) + .filter(|c| !c.is_empty()); + let persist_in_content = persist_choices.is_some(); + let mut options = Vec::new(); + let mut seen = std::collections::HashSet::new(); + if let Some(choices) = persist_choices { + for c in choices { + if options.len() == MAX_OPTIONS { + break; + } + let value = c.value.trim(); + if value.is_empty() + || value == ELICITATION_DECLINE_OPTION_ID + || !seen.insert(value.to_string()) + { + continue; + } + options.push(ElicitationApprovalOption { + option_id: value.to_string(), + label: if c.label.trim().is_empty() { + value.to_string() + } else { + c.label.trim().chars().take(MAX_QUESTION_TEXT_CHARS).collect() + }, + kind: if value == "once" { + "allow_once" + } else { + "allow_always" + }, + }); + } + } + if options.is_empty() { + options.push(ElicitationApprovalOption { + option_id: "accept".to_string(), + label: "Allow".to_string(), + kind: "allow_once", + }); + } + options.push(decline_approval_option()); + ElicitationApproval { + message, + tool_call_id, + options, + persist_in_content, + } +} + +/// True when the raw schema property carries codex's secret marker +/// (`_meta.codex.isSecret`). The typed sacp property structs drop `_meta`, so +/// this reads the raw JSON alongside them. +fn is_secret_property(raw: &Value, id: &str) -> bool { + raw.get("requestedSchema") + .and_then(|s| s.get("properties")) + .and_then(|p| p.get(id)) + .and_then(|prop| prop.get("_meta")) + .and_then(|m| m.get("codex")) + .and_then(|c| c.get("isSecret")) + .and_then(Value::as_bool) + .unwrap_or(false) +} + +/// Convert a form's schema properties into ask-card questions + their typed +/// rebuild plans (see [`classify_elicitation`] for the shape taxonomy). A +/// property with no usable choices renders as free text (the card's +/// always-present "Other" input) — including plain strings, numbers, integers, +/// and choice fields whose options were all empty/duplicate. +fn parse_form_questions( + form: &sacp::schema::ElicitationFormMode, + raw: &Value, +) -> ElicitationQuestions { + let mut specs = Vec::new(); + let mut fields = Vec::new(); + for (id, prop) in &form.requested_schema.properties { + if specs.len() == MAX_QUESTIONS { + tracing::warn!( + "[codex elicitation] dropping question(s) past the max of {MAX_QUESTIONS}" + ); + break; + } + // Skip codex's synthetic free-text "Other" companion fields. + if is_other_companion(id) { + continue; + } + let (title, description, kind, multi_select, choices) = match prop { + ElicitationPropertySchema::String(s) => ( + s.title.clone(), + s.description.clone(), + ElicitationFieldKind::Text, + false, + string_prop_choices(s), + ), + ElicitationPropertySchema::Array(a) => ( + a.title.clone(), + a.description.clone(), + ElicitationFieldKind::MultiSelect, + true, + multi_select_choices(&a.items), + ), + ElicitationPropertySchema::Boolean(b) => ( + b.title.clone(), + b.description.clone(), + ElicitationFieldKind::Boolean, + false, + vec![ + ElicitationChoice { + label: "Yes".to_string(), + value: "true".to_string(), + }, + ElicitationChoice { + label: "No".to_string(), + value: "false".to_string(), + }, + ], + ), + ElicitationPropertySchema::Number(n) => ( + n.title.clone(), + n.description.clone(), + ElicitationFieldKind::Number, + false, + Vec::new(), + ), + ElicitationPropertySchema::Integer(i) => ( + i.title.clone(), + i.description.clone(), + ElicitationFieldKind::Integer, + false, + Vec::new(), + ), + // `ElicitationPropertySchema` is `#[non_exhaustive]`; a future + // property type has no rendering here, so skip it (the agent + // reads the missing key as unanswered and proceeds). + _ => continue, + }; + let question = description + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + .or_else(|| title.as_deref().map(str::trim).filter(|s| !s.is_empty())) + .unwrap_or(id.as_str()); + // Dedup + clamp option labels exactly like the grok bridge, keeping + // the label→value map for the response rebuild. + let mut options = Vec::with_capacity(choices.len().min(MAX_OPTIONS)); + let mut value_by_label = std::collections::HashMap::new(); + let mut seen = std::collections::HashSet::new(); + for c in &choices { + if options.len() == MAX_OPTIONS { + break; + } + let label = c.label.trim(); + if label.is_empty() || !seen.insert(label.to_string()) { + continue; + } + let label: String = label.chars().take(MAX_QUESTION_TEXT_CHARS).collect(); + value_by_label.insert(label.clone(), c.value.clone()); + options.push(QuestionOption { + label, + description: String::new(), + }); + } + let header = title + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(|h| h.chars().take(MAX_HEADER_CHARS).collect()) + .unwrap_or_else(|| synthesize_grok_header(question)); + specs.push(QuestionSpec { + id: id.clone(), + question: question.chars().take(MAX_QUESTION_TEXT_CHARS).collect(), + header, + multi_select, + options, + is_secret: is_secret_property(raw, id), + }); + fields.push(ElicitationField { + id: id.clone(), + kind, + value_by_label, + }); + } + // `tool_call_id` is filled in by `classify_elicitation` (which reads the + // form scope); the parser itself only shapes the questions. + ElicitationQuestions { + specs, + fields, + tool_call_id: None, + } +} + +/// "Yes"/"No" answers (and free-text variants) for a boolean field. +fn parse_bool_answer(v: &str) -> Option { + match v.trim().to_ascii_lowercase().as_str() { + "true" | "yes" | "y" => Some(true), + "false" | "no" | "n" => Some(false), + _ => None, + } +} + +/// Serialize a resolved [`QuestionOutcome`] into the elicitation response. +/// The agent keys answers by the schema property id, which [`QuestionOutcome`] +/// drops — so correlate each answered item back to its spec by +/// `(question, header)` (unique per ask) and rebuild the value through the +/// parallel [`ElicitationField`]: chosen labels map back to their wire values +/// (free-text "Other" answers ride verbatim), typed per the field's kind — an +/// answer that can't be coerced (e.g. non-numeric text for a number field) is +/// omitted rather than sent mistyped. A declined card or an empty result maps +/// to `Decline`, which the agent reads as "no answer" and proceeds — never +/// worse than the pre-bridge behavior. +pub fn build_elicitation_response( + questions: &ElicitationQuestions, + outcome: &QuestionOutcome, +) -> CreateElicitationResponse { + if outcome.declined { + return elicitation_decline_response(); + } + let mut content: BTreeMap = BTreeMap::new(); + for item in &outcome.answers { + let Some(idx) = questions + .specs + .iter() + .position(|s| s.question == item.question && s.header == item.header) + else { + continue; + }; + let field = &questions.fields[idx]; + let mapped: Vec = item + .selected + .iter() + .map(|l| field.value_by_label.get(l).cloned().unwrap_or_else(|| l.clone())) + .collect(); + let value = match field.kind { + ElicitationFieldKind::Text => match mapped.into_iter().next() { + Some(v) => ElicitationContentValue::String(v), + None => continue, + }, + ElicitationFieldKind::MultiSelect => ElicitationContentValue::StringArray(mapped), + ElicitationFieldKind::Boolean => { + match mapped.first().and_then(|v| parse_bool_answer(v)) { + Some(b) => ElicitationContentValue::Boolean(b), + None => continue, + } + } + ElicitationFieldKind::Number => { + match mapped + .first() + .and_then(|v| v.trim().parse::().ok()) + .filter(|f| f.is_finite()) + { + Some(f) => ElicitationContentValue::Number(f), + None => continue, + } + } + ElicitationFieldKind::Integer => { + match mapped.first().and_then(|v| v.trim().parse::().ok()) { + Some(i) => ElicitationContentValue::Integer(i), + None => continue, + } + } + }; + content.insert(field.id.clone(), value); + } + if content.is_empty() { + return elicitation_decline_response(); + } + CreateElicitationResponse::new(ElicitationAction::Accept( + ElicitationAcceptAction::new().content(content), + )) +} + +/// Serialize the user's permission-card choice for an approval-style +/// elicitation. The decline option (and any unknown option id, defensively) +/// maps to `Decline`; an allow choice maps to `Accept`, echoing the chosen +/// persist value back as `content.persist` when the request carried the +/// persist property (codex-acp pops it into the app-server `_meta.persist`; +/// a plain Accept sends no content, which codex-acp reads the same as +/// `content: null`). +pub fn build_elicitation_approval_response( + approval: &ElicitationApproval, + option_id: &str, +) -> CreateElicitationResponse { + let accepted = option_id != ELICITATION_DECLINE_OPTION_ID + && approval.options.iter().any(|o| o.option_id == option_id); + if !accepted { + return elicitation_decline_response(); + } + let mut accept = ElicitationAcceptAction::new(); + if approval.persist_in_content { + let mut content: BTreeMap = BTreeMap::new(); + content.insert( + "persist".to_string(), + ElicitationContentValue::String(option_id.to_string()), + ); + accept = accept.content(content); + } + CreateElicitationResponse::new(ElicitationAction::Accept(accept)) +} + +/// The elicitation reply for a declined / unrenderable / torn-down ask. +/// `Decline` makes the agent proceed with its own judgment (it reads it as no +/// answer), so no path through the bridge can regress the pre-bridge behavior. +pub fn elicitation_decline_response() -> CreateElicitationResponse { + CreateElicitationResponse::new(ElicitationAction::Decline) +} + +/// The elicitation reply when the pending card is torn down without a user +/// choice (turn cancel, disconnect): `Cancel`, mirroring codex-acp's own +/// "cancelled" outcome mapping. +pub fn elicitation_cancel_response() -> CreateElicitationResponse { + CreateElicitationResponse::new(ElicitationAction::Cancel) +} + /// Build the `raw_input` (questions) for the in-stream `AskQuestionResultCard` -/// codeg synthesizes for a grok native ask. Grok never emits a *completed* tool -/// result into the ACP `updates.jsonl` stream (it resolves the answer over the -/// `_x.ai/ask_user_question` ext round-trip instead), so the connection handler -/// emits this itself once the user submits — see `handle_grok_ask_user_question`. +/// codeg synthesizes for a native ask that resolves out-of-band rather than as a +/// completed stream tool_call. Two callers: grok (answers over the +/// `_x.ai/ask_user_question` ext round-trip — `handle_grok_ask_user_question`) +/// and codex `request_user_input` (answers over the `elicitation/create` +/// round-trip — `handle_elicitation_request`). Neither emits a completed tool +/// result into the ACP stream, so the connection handler emits this once the +/// user submits. /// -/// Deliberately omits `header`: grok's native questions have none (the chip -/// header we synthesize for the interactive card is an internal detail), so the -/// frontend parses `header:""` here. The paired [`grok_result_card_output`] uses -/// the SAME empty header, and the history parser (`grok.rs`) emits `header:""` -/// too, so the card's answer↔question match key (`header + question`) aligns and -/// a conversation renders identically live and after reload. Built from the -/// already-clamped [`QuestionSpec`]s so input and output stay in lockstep. +/// Deliberately omits `header`, so the frontend parses `header:""` and matches +/// answers to questions by `question` text alone. The paired +/// [`grok_result_card_output`] uses the SAME empty header. For grok this is exact +/// live↔reload parity (its history parser emits `header:""` too); for codex the +/// reloaded history card (`codex.rs`) does carry the question header, so a +/// multi-question ask shows header tab labels after reload but not live — a +/// cosmetic gap only (single-question asks, the common case, render identically +/// and the answer always shows). Built from the already-clamped [`QuestionSpec`]s +/// so input and output stay in lockstep. pub fn grok_result_card_input(specs: &[QuestionSpec]) -> Value { let questions: Vec = specs .iter() @@ -686,6 +1310,369 @@ mod tests { assert!(!qs[0].id.is_empty()); } + fn elicitation_raw(props: Value, required: Value) -> Value { + json!({ + "mode": "form", + "sessionId": "sess-1", + "toolCallId": "item-1", + "requestedSchema": { + "type": "object", + "properties": props, + "required": required, + }, + "message": "Input requested", + }) + } + + fn expect_questions(plan: ElicitationPlan) -> ElicitationQuestions { + match plan { + ElicitationPlan::Questions(q) => q, + ElicitationPlan::Approval(_) => panic!("expected a questions plan"), + } + } + + fn expect_approval(plan: ElicitationPlan) -> ElicitationApproval { + match plan { + ElicitationPlan::Approval(a) => a, + ElicitationPlan::Questions(_) => panic!("expected an approval plan"), + } + } + + #[test] + fn classify_elicitation_single_select_maps_options_and_id() { + let raw = elicitation_raw( + json!({ + "q1": { + "type": "string", + "title": "Approach", + "description": "Which approach?", + "oneOf": [ + {"const": "Incremental", "title": "Incremental"}, + {"const": "Rewrite", "title": "Rewrite"} + ] + } + }), + json!(["q1"]), + ); + let q = expect_questions(classify_elicitation(&raw).unwrap()); + assert_eq!(q.specs.len(), 1); + assert_eq!(q.specs[0].id, "q1", "property key becomes the spec id"); + assert_eq!(q.specs[0].question, "Which approach?"); + assert_eq!(q.specs[0].header, "Approach"); + assert!(!q.specs[0].multi_select); + assert!(!q.specs[0].is_secret); + let labels: Vec<_> = q.specs[0].options.iter().map(|o| o.label.as_str()).collect(); + assert_eq!(labels, ["Incremental", "Rewrite"]); + assert_eq!(q.fields[0].kind, ElicitationFieldKind::Text); + // The elicitation's toolCallId rides along so the connection handler can + // key the synthesized in-stream result card to it (see the fn docs). + assert_eq!( + q.tool_call_id.as_deref(), + Some("item-1"), + "carries the elicitation toolCallId for the synthesized card" + ); + } + + #[test] + fn classify_elicitation_free_text_renders_and_skips_other_companion() { + // A free-text question (no options) renders as a 0-option spec (the + // card shows its always-present "Other" input); the synthetic + // `__other` companion (any digit-suffixed collision variant too) is + // skipped. + let raw = elicitation_raw( + json!({ + "q1": {"type": "string", "title": "Name", "description": "Your name?", + "_meta": {"codex": {"isSecret": true}}}, + "q1__other": {"type": "string", "title": "Other"}, + "q1__other1": {"type": "string", "title": "Other"} + }), + json!([]), + ); + let q = expect_questions(classify_elicitation(&raw).unwrap()); + assert_eq!(q.specs.len(), 1); + assert_eq!(q.specs[0].id, "q1"); + assert!(q.specs[0].options.is_empty(), "free text has no options"); + assert!(q.specs[0].is_secret, "secret marker read from raw _meta"); + assert!(validate_specs(&q.specs).is_ok(), "0-option specs register"); + + // The typed answer rides the main field verbatim. + let answer = QuestionAnswer { + answers: vec![QuestionAnswerItem { + question_id: "q1".into(), + labels: vec!["Ada Lovelace".into()], + }], + declined: false, + }; + let outcome = build_outcome(&q.specs, &answer); + let v = serde_json::to_value(build_elicitation_response(&q, &outcome)).unwrap(); + assert_eq!(v["action"], "accept"); + assert_eq!(v["content"]["q1"], "Ada Lovelace"); + } + + #[test] + fn classify_elicitation_multi_select_marks_multi_and_maps_values() { + let raw = elicitation_raw( + json!({ + "langs": { + "type": "array", + "title": "Langs", + "description": "Pick languages", + "items": { "anyOf": [ + {"const": "rust", "title": "Rust"}, + {"const": "ts", "title": "TS"} + ]} + } + }), + json!([]), + ); + let q = expect_questions(classify_elicitation(&raw).unwrap()); + assert_eq!(q.specs.len(), 1); + assert!(q.specs[0].multi_select); + // Titles display; consts ride back on accept. + let labels: Vec<_> = q.specs[0].options.iter().map(|o| o.label.as_str()).collect(); + assert_eq!(labels, ["Rust", "TS"]); + let answer = QuestionAnswer { + answers: vec![QuestionAnswerItem { + question_id: q.specs[0].id.clone(), + labels: vec!["Rust".into(), "TS".into()], + }], + declined: false, + }; + let outcome = build_outcome(&q.specs, &answer); + let v = serde_json::to_value(build_elicitation_response(&q, &outcome)).unwrap(); + assert_eq!(v["content"]["langs"], json!(["rust", "ts"])); + } + + #[test] + fn classify_elicitation_boolean_and_numbers_type_their_answers() { + let raw = elicitation_raw( + json!({ + "confirm": {"type": "boolean", "title": "Confirm", "description": "Proceed?"}, + "count": {"type": "integer", "title": "Count", "description": "How many?"}, + "ratio": {"type": "number", "title": "Ratio", "description": "What ratio?"} + }), + json!(["confirm"]), + ); + let q = expect_questions(classify_elicitation(&raw).unwrap()); + assert_eq!(q.specs.len(), 3); + // Booleans render as Yes/No; numbers as free text. + let confirm = q.specs.iter().position(|s| s.id == "confirm").unwrap(); + let labels: Vec<_> = q.specs[confirm].options.iter().map(|o| o.label.as_str()).collect(); + assert_eq!(labels, ["Yes", "No"]); + + let answer = QuestionAnswer { + answers: vec![ + QuestionAnswerItem { + question_id: "confirm".into(), + labels: vec!["Yes".into()], + }, + QuestionAnswerItem { + question_id: "count".into(), + labels: vec!["42".into()], + }, + QuestionAnswerItem { + question_id: "ratio".into(), + labels: vec!["not-a-number".into()], + }, + ], + declined: false, + }; + let outcome = build_outcome(&q.specs, &answer); + let v = serde_json::to_value(build_elicitation_response(&q, &outcome)).unwrap(); + assert_eq!(v["content"]["confirm"], json!(true), "boolean typed"); + assert_eq!(v["content"]["count"], json!(42), "integer typed"); + assert!( + v["content"].get("ratio").is_none(), + "unparseable number omitted, not sent mistyped" + ); + } + + #[test] + fn classify_elicitation_enum_names_display_title_but_send_const() { + // Generic MCP `enum` + `enumNames` normalize to oneOf with + // const ≠ title: display the title, send the const back. + let raw = elicitation_raw( + json!({ + "env": { + "type": "string", + "title": "Env", + "description": "Which environment?", + "oneOf": [ + {"const": "prod-eu-1", "title": "Production (EU)"}, + {"const": "stg-eu-1", "title": "Staging (EU)"} + ] + } + }), + json!(["env"]), + ); + let q = expect_questions(classify_elicitation(&raw).unwrap()); + assert_eq!(q.specs[0].options[0].label, "Production (EU)"); + let answer = QuestionAnswer { + answers: vec![QuestionAnswerItem { + question_id: "env".into(), + labels: vec!["Production (EU)".into()], + }], + declined: false, + }; + let outcome = build_outcome(&q.specs, &answer); + let v = serde_json::to_value(build_elicitation_response(&q, &outcome)).unwrap(); + assert_eq!(v["content"]["env"], "prod-eu-1"); + } + + #[test] + fn classify_elicitation_tool_approval_maps_persist_options() { + // The exact wire shape codex-acp sends for an MCP tool-call approval + // once `elicitation.form` is advertised (verified against its + // elicitation-events tests): message + persist choice, approval marker + // in top-level `_meta`. + let raw = json!({ + "mode": "form", + "sessionId": "sess-1", + "toolCallId": "call-1", + "message": "Allow tool call?", + "requestedSchema": { + "type": "object", + "properties": { + "persist": { + "type": "string", + "title": "Approval scope", + "oneOf": [ + {"const": "once", "title": "Allow once"}, + {"const": "session", "title": "Allow for this session"}, + {"const": "always", "title": "Allow and don't ask again"} + ], + "default": "once" + } + }, + "required": ["persist"] + }, + "_meta": {"codex_approval_kind": "mcp_tool_call", "persist": ["session", "always"]} + }); + let approval = expect_approval(classify_elicitation(&raw).unwrap()); + assert_eq!(approval.message, "Allow tool call?"); + assert_eq!(approval.tool_call_id.as_deref(), Some("call-1")); + assert!(approval.persist_in_content); + let ids: Vec<_> = approval.options.iter().map(|o| o.option_id.as_str()).collect(); + assert_eq!(ids, ["once", "session", "always", ELICITATION_DECLINE_OPTION_ID]); + assert_eq!(approval.options[0].label, "Allow once"); + assert_eq!(approval.options[0].kind, "allow_once"); + assert_eq!(approval.options[1].kind, "allow_always"); + + // Accepting echoes the chosen persist back in content… + let v = + serde_json::to_value(build_elicitation_approval_response(&approval, "session")) + .unwrap(); + assert_eq!(v["action"], "accept"); + assert_eq!(v["content"]["persist"], "session"); + // …declining (or an unknown option) maps to decline. + let v = serde_json::to_value(build_elicitation_approval_response( + &approval, + ELICITATION_DECLINE_OPTION_ID, + )) + .unwrap(); + assert_eq!(v["action"], "decline"); + let v = serde_json::to_value(build_elicitation_approval_response(&approval, "bogus")) + .unwrap(); + assert_eq!(v["action"], "decline"); + } + + #[test] + fn classify_elicitation_tool_approval_without_persist_is_allow_decline() { + // No persist advertised → codex-acp sends an EMPTY properties object. + // This must NOT auto-decline: it renders Allow/Decline and accepts + // with no content. + let raw = json!({ + "mode": "form", + "sessionId": "sess-1", + "toolCallId": "call-1", + "message": "Allow tool call?", + "requestedSchema": {"type": "object", "properties": {}}, + "_meta": {"codex_approval_kind": "mcp_tool_call"} + }); + let approval = expect_approval(classify_elicitation(&raw).unwrap()); + assert!(!approval.persist_in_content); + let ids: Vec<_> = approval.options.iter().map(|o| o.option_id.as_str()).collect(); + assert_eq!(ids, ["accept", ELICITATION_DECLINE_OPTION_ID]); + let v = serde_json::to_value(build_elicitation_approval_response(&approval, "accept")) + .unwrap(); + assert_eq!(v["action"], "accept"); + assert!( + v.get("content").is_none() || v["content"].is_null(), + "plain accept sends no content" + ); + } + + #[test] + fn classify_elicitation_message_only_form_is_accept_decline_confirm() { + // A non-approval form with nothing to fill in (a bare MCP server + // confirmation) renders Accept/Decline rather than auto-declining. + let raw = elicitation_raw(json!({}), json!([])); + let approval = expect_approval(classify_elicitation(&raw).unwrap()); + assert_eq!(approval.message, "Input requested"); + let ids: Vec<_> = approval.options.iter().map(|o| o.option_id.as_str()).collect(); + assert_eq!(ids, ["accept", ELICITATION_DECLINE_OPTION_ID]); + } + + #[test] + fn elicitation_round_trips_answer_keyed_by_property_id() { + let raw = elicitation_raw( + json!({ + "colour": { + "type": "string", + "title": "Colour", + "description": "Pick a colour", + "oneOf": [ + {"const": "Red", "title": "Red"}, + {"const": "Blue", "title": "Blue"} + ] + } + }), + json!(["colour"]), + ); + let q = expect_questions(classify_elicitation(&raw).unwrap()); + // Simulate the user picking "Blue" through the normal answer path. + let answer = QuestionAnswer { + answers: vec![QuestionAnswerItem { + question_id: "colour".into(), + labels: vec!["Blue".into()], + }], + declined: false, + }; + let outcome = build_outcome(&q.specs, &answer); + let resp = build_elicitation_response(&q, &outcome); + let v = serde_json::to_value(&resp).unwrap(); + assert_eq!(v["action"], "accept"); + // Codex reads the answer back by the schema property id. + assert_eq!(v["content"]["colour"], "Blue"); + } + + #[test] + fn build_elicitation_response_declined_maps_to_decline() { + let outcome = QuestionOutcome { + answers: vec![], + declined: true, + }; + let empty = ElicitationQuestions { + specs: vec![], + fields: vec![], + tool_call_id: None, + }; + let v = serde_json::to_value(build_elicitation_response(&empty, &outcome)).unwrap(); + assert_eq!(v["action"], "decline"); + let v = serde_json::to_value(elicitation_cancel_response()).unwrap(); + assert_eq!(v["action"], "cancel"); + } + + #[test] + fn elicitation_auto_resolution_ms_reads_codex_meta() { + let mut raw = elicitation_raw(json!({}), json!([])); + assert_eq!(elicitation_auto_resolution_ms(&raw), None); + raw["_meta"] = json!({"codex": {"autoResolutionMs": 30000}}); + assert_eq!(elicitation_auto_resolution_ms(&raw), Some(30000)); + raw["_meta"] = json!({"codex": {"autoResolutionMs": null}}); + assert_eq!(elicitation_auto_resolution_ms(&raw), None); + } + #[test] fn validate_specs_accepts_well_formed_and_rejects_malformed() { // What parse_questions mints passes the request-side re-check. @@ -709,6 +1696,7 @@ mod tests { description: String::new(), }) .collect(), + is_secret: false, }; assert!(validate_specs(&[]).is_err(), "empty set"); @@ -717,8 +1705,8 @@ mod tests { "oversized question text" ); assert!( - validate_specs(&[spec("ok", MIN_OPTIONS - 1, 0)]).is_err(), - "too few options" + validate_specs(&[spec("ok", 0, 0)]).is_ok(), + "0 options is a legal free-text ask (elicitation / MCP forms)" ); assert!( validate_specs(&[spec("ok", MAX_OPTIONS + 1, 0)]).is_err(), @@ -751,6 +1739,7 @@ mod tests { description: String::new(), }, ], + is_secret: false, }; assert!(validate_specs(&[blank_id]).is_err(), "blank id"); // Duplicate option label within one question (parse_questions rejects it). @@ -769,6 +1758,7 @@ mod tests { description: String::new(), }, ], + is_secret: false, }; assert!( validate_specs(&[dup_label]).is_err(), diff --git a/src-tauri/src/acp/registry.rs b/src-tauri/src/acp/registry.rs index 324d28cbb..485cb8678 100644 --- a/src-tauri/src/acp/registry.rs +++ b/src-tauri/src/acp/registry.rs @@ -209,16 +209,41 @@ pub fn get_agent_meta(agent_type: AgentType) -> AcpAgentMeta { description: "ACP adapter for OpenAI's coding assistant", // codex-acp moved from zed-industries (Rust binary) to the // agentclientprotocol org (TypeScript rewrite, npx-distributed). - // 1.1.2 depends on `@openai/codex` ^0.144.0 and drives `codex + // 1.1.7 depends on `@openai/codex` ^0.145.0 and drives `codex // app-server`; since 1.0.1 it also resolves the resumed // `model_provider` from `~/.codex/config.toml` (#224), so codeg no // longer injects `MODEL_PROVIDER` to keep resumed sessions on the - // custom provider. 1.1.0 (#263) also reports `/goal` transitions as a + // custom provider. 1.1.0 (#263) reports `/goal` transitions as a // structured `session_info_update` (`_meta.codex.goal`) rather than - // live agent text — see `crate::acp::codex_goal`. + // live agent text — see `crate::acp::codex_goal`. 1.1.3+ adds three + // new live signals codeg handles in `connection::emit_conversation_update`: + // `subAgentActivity` tool calls (#304, suppressed via + // `is_codex_subagent_activity` — redundant with the collab capsule), + // retryable turn errors (#289, `_meta.codex.error` → a transient + // retry banner via `codex_retry_indicator`), and the + // context-compaction lifecycle (#288, `_meta.contextCompaction` tool + // call → a dedicated frontend card). 1.1.x also adds Plan mode: the + // `collaboration_mode` config option (rendered by the generic + // config-option path) and native `request_user_input`, delivered as + // an ACP `elicitation/create` request — codeg advertises + // `elicitation.form` for Codex and bridges the WHOLE form surface + // (Plan-mode questions, MCP-server forms, MCP tool-call approvals) + // in `handle_elicitation_request` / `question::classify_elicitation`. + // 1.1.5 (#322) also widened codex-acp's MCP config filtering to + // project `.codex` layers, which is why codeg forces + // `DISABLE_MCP_CONFIG_FILTERING` (see `apply_codex_env_policy`) so + // the injected `codeg-mcp` server always survives. 1.1.6 adds + // steering (#309): `_session/steering` injects a user prompt into + // the LIVE turn (initialize advertises `_meta.steering.supported`) + // — not wired into codeg yet. 1.1.7 (#326) emits Plan-mode plan + // contents as a plain `agent_message_chunk` + // (`_meta.codex.phase = "final_answer"`, no `` tags), + // which the adapter's tag-splitter simply no-ops on — tagged output + // from older codex still renders as the proposed-plan card. 1.1.7 + // declares no `engines.node`, so the 20.0.0 floor is retained. distribution: AgentDistribution::Npx { - version: "1.1.2", - package: "@agentclientprotocol/codex-acp@1.1.2", + version: "1.1.7", + package: "@agentclientprotocol/codex-acp@1.1.7", cmd: "codex-acp", args: &[], env: &[], @@ -639,8 +664,8 @@ mod tests { ); assert_npx_version( AgentType::Codex, - "1.1.2", - "@agentclientprotocol/codex-acp@1.1.2", + "1.1.7", + "@agentclientprotocol/codex-acp@1.1.7", Some("20.0.0"), ); assert_npx_version(AgentType::Pi, "0.0.31", "pi-acp@0.0.31", Some("22.0.0")); diff --git a/src-tauri/src/acp/session_state.rs b/src-tauri/src/acp/session_state.rs index 449bd6e96..f172031e9 100644 --- a/src-tauri/src/acp/session_state.rs +++ b/src-tauri/src/acp/session_state.rs @@ -10,6 +10,7 @@ use serde::{Deserialize, Serialize}; use crate::acp::event_stream::{ConnectionEventStream, RecentEventsBuffer}; use crate::acp::feedback::{FeedbackItem, FeedbackStatus}; +use crate::acp::plan_approval::PendingPlanApprovalState; use crate::acp::question::PendingQuestionState; use crate::acp::types::{ AcpEvent, AvailableCommandInfo, ConfigStaleKind, ConnectionStatus, EventEnvelope, @@ -247,6 +248,16 @@ pub struct SessionState { /// the backend's `pending_questions` registry keys the answer one-shot. pub pending_question: Option, + /// The agent's in-flight Grok `exit_plan_mode` approval (the plan awaiting the + /// user's Approve / Request-changes / Abandon decision). Set by + /// `PlanApprovalRequest`, cleared by a matching `PlanApprovalResolved` (and + /// defensively on `TurnComplete`). Carried on `to_snapshot()` so a client + /// attaching mid-turn re-renders the approval card the one-shot event won't + /// replay for it. At most one is pending (the agent is blocked in its + /// `exit_plan_mode` tool call); the connection parks the ext responder keyed + /// by `approval_id`. + pub pending_plan_approval: Option, + /// In-flight (running) sub-agent delegations keyed by `parent_tool_use_id`. /// `DelegationStarted` inserts; `DelegationCompleted` removes. UNLIKE /// `active_tool_calls`, NOT cleared on `TurnComplete` (an async delegation @@ -446,6 +457,7 @@ impl SessionState { active_tool_calls: BTreeMap::new(), pending_permission: None, pending_question: None, + pending_plan_approval: None, active_delegations: BTreeMap::new(), feedback: Vec::new(), background_outstanding: 0, @@ -713,6 +725,29 @@ impl SessionState { self.pending_question = None; } } + AcpEvent::PlanApprovalRequest { + approval_id, + tool_call_id, + plan_markdown, + } => { + self.pending_plan_approval = Some(PendingPlanApprovalState { + approval_id: approval_id.clone(), + tool_call_id: tool_call_id.clone(), + plan_markdown: plan_markdown.clone(), + created_at: Utc::now(), + }); + } + AcpEvent::PlanApprovalResolved { approval_id } => { + // Mirror `QuestionResolved`: only clear when the resolved id + // matches the current one, so a late event for an already- + // replaced approval can't wipe a live card from under the user. + if matches!( + &self.pending_plan_approval, + Some(p) if p.approval_id == *approval_id, + ) { + self.pending_plan_approval = None; + } + } AcpEvent::TurnComplete { stop_reason, .. } => { // Diagnostic only (no behavior change): pairs with the // StatusChanged log above. This is the ACTUAL point the turn @@ -788,6 +823,10 @@ impl SessionState { // answer one-shot is cleaned via the listener's peer-close race; // this just keeps the snapshot honest. self.pending_question = None; + // Likewise a blocked `exit_plan_mode` approval: the parked ext + // responder is drained by the connection's teardown/cancel path; + // this just keeps the snapshot honest if the turn settles first. + self.pending_plan_approval = None; self.status = ConnectionStatus::Connected; } AcpEvent::UserMessage { message_id, blocks } => { @@ -820,6 +859,11 @@ impl SessionState { self.feedback.clear(); // A new user turn supersedes any stale pending question. self.pending_question = None; + // Likewise a stale plan approval: a new turn started without a + // clean TurnComplete (fork/resume re-prompt, error recovery, or a + // queued prompt sent instead of answering) must not leave a dead + // approval in the snapshot for a mid-turn attach to render. + self.pending_plan_approval = None; } AcpEvent::ConversationLinked { conversation_id, @@ -939,9 +983,12 @@ impl SessionState { } AcpEvent::ClaudeSdkMessage { .. } | AcpEvent::SessionLoadFailed { .. } + | AcpEvent::TurnRetrying { .. } | AcpEvent::UserPromptSent { .. } => { // 这些事件不直接修改 SessionState 的可见字段。 // UserPromptSent 是纯通知事件，仅供 chat-channel 推送消费。 + // TurnRetrying 与 Claude 的 api_retry 一样是前端瞬态提示（重试横幅）， + // 不进快照——回合边界会清除它。 } } self.last_activity_at = Utc::now(); @@ -1214,6 +1261,7 @@ impl SessionState { active_tool_calls: self.active_tool_calls.values().cloned().collect(), pending_permission: self.pending_permission.clone(), pending_question: self.pending_question.clone(), + pending_plan_approval: self.pending_plan_approval.clone(), pending_user_message: self.pending_user_message.clone(), active_delegations: self.active_delegations.values().cloned().collect(), feedback: self.feedback.clone(), @@ -1269,6 +1317,13 @@ pub struct LiveSessionSnapshot { /// the wire so every snapshot stays byte-identical with the pre-feature shape. #[serde(default, skip_serializing_if = "Option::is_none")] pub pending_question: Option, + /// The agent's in-flight Grok `exit_plan_mode` approval (see + /// `SessionState.pending_plan_approval`). `#[serde(default)]` so older + /// payloads deserialize; `skip_serializing_if` keeps the common no-approval + /// case off the wire so every snapshot stays byte-identical with the + /// pre-feature shape. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pending_plan_approval: Option, /// The in-flight user prompt for the current turn (see /// `SessionState.pending_user_message`). `#[serde(default)]` so older /// payloads still deserialize; `skip_serializing_if` so the no-pending case @@ -1433,6 +1488,71 @@ mod tests { ) } + #[test] + fn plan_approval_applies_clears_by_id_and_survives_snapshot() { + let mut s = fresh_state(); + // Request → pending set + carried on the snapshot for mid-turn attach. + s.apply_event(&AcpEvent::PlanApprovalRequest { + approval_id: "ap-1".into(), + tool_call_id: "call-1".into(), + plan_markdown: "# Plan".into(), + }); + let pending = s.pending_plan_approval.clone().expect("pending set"); + assert_eq!(pending.approval_id, "ap-1"); + assert_eq!(pending.tool_call_id, "call-1"); + assert_eq!(pending.plan_markdown, "# Plan"); + assert!(s.to_snapshot().pending_plan_approval.is_some()); + + // A resolve for a DIFFERENT id must not wipe the live approval. + s.apply_event(&AcpEvent::PlanApprovalResolved { + approval_id: "other".into(), + }); + assert!(s.pending_plan_approval.is_some()); + + // Matching resolve clears it (and the snapshot). + s.apply_event(&AcpEvent::PlanApprovalResolved { + approval_id: "ap-1".into(), + }); + assert!(s.pending_plan_approval.is_none()); + assert!(s.to_snapshot().pending_plan_approval.is_none()); + } + + #[test] + fn turn_complete_clears_pending_plan_approval() { + let mut s = fresh_state(); + s.apply_event(&AcpEvent::PlanApprovalRequest { + approval_id: "ap-1".into(), + tool_call_id: "c".into(), + plan_markdown: String::new(), + }); + assert!(s.pending_plan_approval.is_some()); + s.apply_event(&AcpEvent::TurnComplete { + session_id: "sid".into(), + stop_reason: "end_turn".into(), + agent_type: "grok".into(), + }); + assert!(s.pending_plan_approval.is_none()); + } + + #[test] + fn user_message_supersedes_stale_pending_plan_approval() { + // A new turn starting without a clean TurnComplete (fork/resume re-prompt, + // queued prompt sent instead of answering) must not leave a dead approval + // in the snapshot for a mid-turn attach to render. + let mut s = fresh_state(); + s.apply_event(&AcpEvent::PlanApprovalRequest { + approval_id: "ap-1".into(), + tool_call_id: "c".into(), + plan_markdown: "# plan".into(), + }); + assert!(s.pending_plan_approval.is_some()); + s.apply_event(&AcpEvent::UserMessage { + message_id: "m1".into(), + blocks: vec![], + }); + assert!(s.pending_plan_approval.is_none()); + } + #[test] fn background_activity_mirrors_outstanding_and_gates_keepalive() { let mut s = fresh_state(); diff --git a/src-tauri/src/acp/types.rs b/src-tauri/src/acp/types.rs index 925220f18..bb4d67b05 100644 --- a/src-tauri/src/acp/types.rs +++ b/src-tauri/src/acp/types.rs @@ -200,6 +200,21 @@ pub enum AcpEvent { #[serde(skip, default)] terminal: bool, }, + /// A retryable turn error that keeps the turn alive (codex-acp #289, + /// v1.1.3+). Codex reports a transient, auto-retried error as + /// `session_info_update._meta.codex.error` (only when `willRetry == true`) + /// and continues the turn rather than terminating it. Surfaced as a + /// transient "retrying" indicator on the active turn — it is NOT a turn + /// failure and must not be rendered as one. The frontend reuses the Claude + /// API-retry banner and clears it at the next turn boundary. + TurnRetrying { + /// Human-readable transient error (`_meta.codex.error.message`). + message: String, + /// HTTP status pulled from a `codexErrorInfo` object variant + /// (e.g. `responseStreamDisconnected.httpStatusCode`), when present. + #[serde(skip_serializing_if = "Option::is_none")] + error_status: Option, + }, /// `session/load` failed in a non-recoverable way (e.g. the agent has no /// record of this `session_id`). Emitted instead of silently falling back /// to `session/new`, so the frontend can surface the failure with reload @@ -331,6 +346,23 @@ pub enum AcpEvent { /// (the tool call was aborted / the connection drained). Carries only the /// `question_id`; clients clear the matching card. Idempotent on apply. QuestionResolved { question_id: String }, + /// A Grok `exit_plan_mode` call: the agent finished planning and is BLOCKED + /// on the user's approval of the plan before it leaves plan mode and starts + /// implementing (Grok's native `_x.ai/exit_plan_mode` ext request). Broadcast + /// so every client viewing this conversation renders the interactive + /// plan-approval card above the input box, and captured into + /// `SessionState.pending_plan_approval` so a client attaching mid-turn (cold + /// attach, reconnect, another window) recovers it from the snapshot. The + /// backend parks the blocked ext-request responder keyed by `approval_id`. + PlanApprovalRequest { + approval_id: String, + tool_call_id: String, + plan_markdown: String, + }, + /// A previously-pending plan approval was answered (from any client) or + /// canceled (the connection drained). Carries only the `approval_id`; clients + /// clear the matching card. Idempotent on apply. + PlanApprovalResolved { approval_id: String }, /// The agent's effective settings (env vars / model provider / native config /// files) changed AFTER this connection was spawned, so the running process /// is still using its launch-time config. Emitted by diff --git a/src-tauri/src/app_state.rs b/src-tauri/src/app_state.rs index 7d43d873e..dea9673b0 100644 --- a/src-tauri/src/app_state.rs +++ b/src-tauri/src/app_state.rs @@ -170,6 +170,11 @@ pub fn build_delegation_stack( questions: Arc::new(crate::acp::manager::ConnectionManagerQuestionLookup { manager: Arc::new(connection_manager.clone_ref()), }) as Arc, + // Grok `exit_plan_mode` bridge — always wired (native plan mode, no + // feature flag), same backing manager as the question lookup. + plan_approvals: Arc::new(crate::acp::manager::ConnectionManagerPlanApprovalLookup { + manager: Arc::new(connection_manager.clone_ref()), + }) as Arc, }); (broker, tokens, socket_path, feedback, ask, sessions) diff --git a/src-tauri/src/chat_channel/event_subscriber.rs b/src-tauri/src/chat_channel/event_subscriber.rs index eb901d7e4..d9eaaa962 100644 --- a/src-tauri/src/chat_channel/event_subscriber.rs +++ b/src-tauri/src/chat_channel/event_subscriber.rs @@ -632,6 +632,7 @@ mod permission_push_tests { description: String::new(), }, ], + is_secret: false, }], }, } diff --git a/src-tauri/src/chat_channel/message_formatter.rs b/src-tauri/src/chat_channel/message_formatter.rs index aea7f08eb..3334606cd 100644 --- a/src-tauri/src/chat_channel/message_formatter.rs +++ b/src-tauri/src/chat_channel/message_formatter.rs @@ -226,6 +226,7 @@ mod question_request_tests { description: String::new(), }) .collect(), + is_secret: false, } } diff --git a/src-tauri/src/commands/acp.rs b/src-tauri/src/commands/acp.rs index ca58260d0..a462f2bb6 100644 --- a/src-tauri/src/commands/acp.rs +++ b/src-tauri/src/commands/acp.rs @@ -7607,6 +7607,16 @@ pub async fn acp_set_config_option( .await } +#[cfg(feature = "tauri-runtime")] +#[cfg_attr(feature = "tauri-runtime", tauri::command)] +pub async fn acp_goal_control( + connection_id: String, + action: crate::acp::connection::GoalControlAction, + manager: State<'_, ConnectionManager>, +) -> Result<(), AcpError> { + manager.goal_control(&connection_id, action).await +} + /// Spawn a transient ACP connection for `agent_type` with a silent emitter, /// read whatever `SessionConfigOptions` / `SessionModes` the agent advertises, /// and tear it down. The returned snapshot drives the delegation-settings UI @@ -7702,6 +7712,19 @@ pub async fn acp_answer_question( .await } +#[cfg(feature = "tauri-runtime")] +#[cfg_attr(feature = "tauri-runtime", tauri::command)] +pub async fn acp_answer_plan_approval( + connection_id: String, + approval_id: String, + answer: crate::acp::plan_approval::PlanApprovalAnswer, + manager: State<'_, ConnectionManager>, +) -> Result<(), AcpError> { + manager + .answer_plan_approval(&connection_id, &approval_id, answer) + .await +} + #[cfg(feature = "tauri-runtime")] #[cfg_attr(feature = "tauri-runtime", tauri::command)] pub async fn acp_disconnect( diff --git a/src-tauri/src/db/service/import_service.rs b/src-tauri/src/db/service/import_service.rs index 476f751f6..bba9510f3 100644 --- a/src-tauri/src/db/service/import_service.rs +++ b/src-tauri/src/db/service/import_service.rs @@ -288,7 +288,10 @@ async fn import_one( title: Set(summary.title.clone()), title_locked: Set(false), agent_type: Set(at_str), - status: Set(conversation::ConversationStatus::Completed), + // Imported sessions land as `PendingReview` ("待审查"), not `Completed`: + // they are surfaced for the user to review and stay visible even when + // the sidebar's "show completed" filter is off (now its default). + status: Set(conversation::ConversationStatus::PendingReview), // Imports scan regular folders' session files; chat scratch dirs and // loop runs are never import targets, so every imported row is regular. kind: Set(conversation::ConversationKind::Regular), @@ -368,6 +371,30 @@ mod tests { assert!(!got.title_locked, "auto refresh must not lock the title"); } + #[tokio::test] + async fn import_inserts_a_pending_review_conversation() { + let db = fresh_in_memory_db().await; + let folder = seed_folder(&db, "/tmp/codeg-import-status").await; + let at = AgentType::ClaudeCode; + + assert_eq!( + import_one(&db.conn, folder, &at, &summary("ext-1", Some("first prompt"))) + .await + .expect("import"), + ImportOutcome::Imported + ); + + let id = find_id(&db.conn, "ext-1").await; + let got = conversation_service::get_by_id(&db.conn, id) + .await + .expect("get"); + // Imported rows are surfaced for review, not marked done — so they stay + // visible even when the sidebar hides completed conversations. + // `DbConversationSummary.status` is the serde-serialized string + // (`rename_all = "snake_case"`), not the enum. + assert_eq!(got.status, "pending_review"); + } + #[tokio::test] async fn reimport_skips_an_unchanged_title() { let db = fresh_in_memory_db().await; diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 9e48efa3c..157ce5cde 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1,3 +1,12 @@ +// The ACP connection driver (`acp::connection`) wraps the enormous +// `run_connection` future in a `block_on(async move { … })` frame whose type +// layout nests deep enough to blow rustc's default query depth of 128 (it +// overflowed by ~130 when computing the async block's layout). This is a +// compile-time type-recursion knob, unrelated to any runtime limit — bump it +// so the giant future's layout resolves. See the big-stack thread in +// `acp/connection.rs` for the sibling *runtime* mitigation of the same frame. +#![recursion_limit = "256"] + pub mod acp; pub use acp::{ idle_sweep_task, idle_timeout_from_env, lifecycle_subscriber_task, SWEEP_INTERVAL_SECS, @@ -1088,11 +1097,13 @@ mod tauri_app { acp_commands::acp_prompt, acp_commands::acp_set_mode, acp_commands::acp_set_config_option, + acp_commands::acp_goal_control, acp_commands::acp_describe_agent_options, acp_commands::acp_cancel, acp_commands::acp_fork, acp_commands::acp_respond_permission, acp_commands::acp_answer_question, + acp_commands::acp_answer_plan_approval, acp_commands::acp_disconnect, acp_commands::acp_touch_connection, acp_commands::acp_list_connections, diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 49abbdb6e..4d030b22e 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -894,10 +894,13 @@ impl CodexParser { // - close_agent → folded into the execution capsule (no own capsule); // its result is only a fallback for agents never waited on. // codex-acp 1.0.1 (#223) maps `collabAgentToolCall` onto live ACP - // `tool_call`s and still drops `subAgentActivity`, so the nested - // `agent-.jsonl` stats only exist on history reload. Live and - // reconstructed capsules never double-render (live during streaming, - // this on reload). + // `tool_call`s; 1.1.3+ (#304) additionally emits `subAgentActivity` as a + // SEPARATE live `tool_call` (`_meta.codex.subagent`), but codeg + // suppresses it (redundant with the collab capsule — see + // `is_codex_subagent_activity`) and it carries no transcript content, so + // the nested `agent-.jsonl` stats still only exist on history reload. + // Live and reconstructed capsules never double-render (live during + // streaming, this on reload). let mut spawn_agent_call_ids: HashSet = HashSet::new(); let mut agent_id_to_spawn_call_id: HashMap = HashMap::new(); // Result text used to FILL the execution capsule only as a fallback for diff --git a/src-tauri/src/web/handlers/acp.rs b/src-tauri/src/web/handlers/acp.rs index 2a536b992..d9c3ab502 100644 --- a/src-tauri/src/web/handlers/acp.rs +++ b/src-tauri/src/web/handlers/acp.rs @@ -365,6 +365,25 @@ pub async fn acp_set_config_option( Ok(Json(())) } +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct AcpGoalControlParams { + pub connection_id: String, + pub action: crate::acp::connection::GoalControlAction, +} + +pub async fn acp_goal_control( + Extension(state): Extension>, + Json(params): Json, +) -> Result, AppCommandError> { + let manager = &state.connection_manager; + manager + .goal_control(¶ms.connection_id, params.action) + .await + .map_err(|e| AppCommandError::task_execution_failed(e.to_string()))?; + Ok(Json(())) +} + #[derive(Deserialize)] #[serde(rename_all = "camelCase")] pub struct AcpDescribeAgentOptionsParams { @@ -469,6 +488,26 @@ pub async fn acp_answer_question( Ok(Json(())) } +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct AcpAnswerPlanApprovalParams { + pub connection_id: String, + pub approval_id: String, + pub answer: crate::acp::plan_approval::PlanApprovalAnswer, +} + +pub async fn acp_answer_plan_approval( + Extension(state): Extension>, + Json(params): Json, +) -> Result, AppCommandError> { + let manager = &state.connection_manager; + manager + .answer_plan_approval(¶ms.connection_id, ¶ms.approval_id, params.answer) + .await + .map_err(|e| AppCommandError::task_execution_failed(e.to_string()))?; + Ok(Json(())) +} + pub async fn acp_list_connections( Extension(state): Extension>, ) -> Result>, AppCommandError> { diff --git a/src-tauri/src/web/router.rs b/src-tauri/src/web/router.rs index 249d1b17f..930fe4c28 100644 --- a/src-tauri/src/web/router.rs +++ b/src-tauri/src/web/router.rs @@ -614,6 +614,10 @@ pub fn build_router( "/acp_set_config_option", post(handlers::acp::acp_set_config_option), ) + .route( + "/acp_goal_control", + post(handlers::acp::acp_goal_control), + ) .route( "/acp_describe_agent_options", post(handlers::acp::acp_describe_agent_options), @@ -628,6 +632,10 @@ pub fn build_router( "/acp_answer_question", post(handlers::acp::acp_answer_question), ) + .route( + "/acp_answer_plan_approval", + post(handlers::acp::acp_answer_plan_approval), + ) .route( "/acp_list_connections", post(handlers::acp::acp_list_connections), diff --git a/src-tauri/tests/delegation_e2e_uds.rs b/src-tauri/tests/delegation_e2e_uds.rs index 703ef8bd3..13a9814f7 100644 --- a/src-tauri/tests/delegation_e2e_uds.rs +++ b/src-tauri/tests/delegation_e2e_uds.rs @@ -466,6 +466,7 @@ async fn end_to_end_uds_ask_question_round_trip() { description: String::new(), }, ], + is_secret: false, }], }; client_ask_round_trip(&socket_str, &req).await @@ -601,6 +602,7 @@ async fn end_to_end_uds_ask_revoked_after_register_declines() { description: String::new(), }, ], + is_secret: false, }], }; let resp = client_ask_round_trip(&socket.to_string_lossy(), &req) diff --git a/src/components/ai-elements/message.tsx b/src/components/ai-elements/message.tsx index 5edc19f31..451f47cd3 100644 --- a/src/components/ai-elements/message.tsx +++ b/src/components/ai-elements/message.tsx @@ -40,10 +40,14 @@ export type MessageProps = HTMLAttributes & { export const Message = ({ className, from, ...props }: MessageProps) => (
{ }) }) + it("renders a free-text question (no options) as a bare input and submits it", () => { + // Codex elicitation / MCP-server forms ask open questions with 0 options: + // the input is the answer field — no "Other" toggle to click through. + const freeText: PendingQuestionState = { + question_id: "q-free", + created_at: "2026-01-01T00:00:00Z", + questions: [ + { + id: "qf", + question: "What is the base URL?", + header: "URL", + multi_select: false, + options: [], + }, + ], + } + const onAnswer = renderCard(freeText) + expect(screen.queryByRole("radio")).toBeNull() + const input = screen.getByPlaceholderText("Type your answer…") + const submit = screen.getByRole("button", { name: "Submit" }) + expect(submit).toBeDisabled() + fireEvent.change(input, { target: { value: "https://api.example.com" } }) + expect(submit).toBeEnabled() + fireEvent.click(submit) + expect(onAnswer).toHaveBeenCalledWith("q-free", { + answers: [{ questionId: "qf", labels: ["https://api.example.com"] }], + declined: false, + }) + }) + + it("masks the input for a secret question", () => { + const secret: PendingQuestionState = { + question_id: "q-secret", + created_at: "2026-01-01T00:00:00Z", + questions: [ + { + id: "qs", + question: "Paste your API key", + header: "Key", + multi_select: false, + options: [], + is_secret: true, + }, + ], + } + renderCard(secret) + const input = screen.getByPlaceholderText("Type your answer…") + expect(input).toHaveAttribute("type", "password") + }) + it("skips with a declined answer", () => { const onAnswer = renderCard(single) fireEvent.click(screen.getByRole("button", { name: "Skip" })) diff --git a/src/components/chat/ask-question-card.tsx b/src/components/chat/ask-question-card.tsx index bd081c938..9caf679ca 100644 --- a/src/components/chat/ask-question-card.tsx +++ b/src/components/chat/ask-question-card.tsx @@ -302,17 +302,42 @@ export function AskQuestionCard({ const renderOptions = (q: QuestionSpec) => { const s = state[q.id] const otherId = `${q.id}-other` + // Secret questions (codex marks API keys etc. with `is_secret`) mask the + // typed answer. + const inputType = q.is_secret ? "password" : "text" + const inputClass = + "w-full rounded-md border border-border/60 bg-background px-2.5 py-1.5 text-sm outline-none focus:border-ring disabled:cursor-not-allowed disabled:opacity-60" + + // Free-text question (no options — codex elicitation and MCP-server forms + // ask open questions this way): the text input IS the answer field, always + // visible, with no "Other" toggle to click through first. Typing marks the + // question answered (`setOtherText` activates the free-text state). + if (q.options.length === 0) { + return ( + setOtherText(q, e.target.value)} + placeholder={t("otherPlaceholder")} + className={inputClass} + /> + ) + } + const otherInput = s?.otherActive ? ( setOtherText(q, e.target.value)} placeholder={t("otherPlaceholder")} - className="w-full rounded-md border border-border/60 bg-background px-2.5 py-1.5 text-sm outline-none focus:border-ring disabled:cursor-not-allowed disabled:opacity-60" + className={inputClass} /> ) : null diff --git a/src/components/chat/conversation-shell.tsx b/src/components/chat/conversation-shell.tsx index c976af0bf..2ccf9775a 100644 --- a/src/components/chat/conversation-shell.tsx +++ b/src/components/chat/conversation-shell.tsx @@ -3,7 +3,9 @@ import { useTranslations } from "next-intl" import type { AgentType, ConnectionStatus, + PendingPlanApprovalState, PendingQuestionState, + PlanApprovalAnswer, PromptCapabilitiesInfo, PromptDraft, PromptInputBlock, @@ -23,6 +25,7 @@ import { ChatInput } from "@/components/chat/chat-input" import { PermissionDialog } from "@/components/chat/permission-dialog" import { QuestionDialog } from "@/components/chat/question-dialog" import { AskQuestionCard } from "@/components/chat/ask-question-card" +import { PlanApprovalCard } from "@/components/chat/plan-approval-card" interface ConversationShellProps { status: ConnectionStatus | null @@ -35,6 +38,8 @@ interface ConversationShellProps { pendingQuestion: PendingQuestion | null /** Awaiting-answer multiple-choice `ask_user_question`. */ pendingAskQuestion: PendingQuestionState | null + /** Awaiting-decision Grok `exit_plan_mode` approval. */ + pendingPlanApproval: PendingPlanApprovalState | null onFocus: () => void onSend: (draft: PromptDraft, modeId?: string | null) => void onCancel: () => void @@ -44,6 +49,10 @@ interface ConversationShellProps { questionId: string, answer: QuestionAnswer ) => void | Promise + onAnswerPlanApproval: ( + approvalId: string, + answer: PlanApprovalAnswer + ) => void | Promise children: ReactNode modes?: SessionModeInfo[] configOptions?: SessionConfigOptionInfo[] @@ -99,12 +108,14 @@ export function ConversationShell({ pendingPermission, pendingQuestion, pendingAskQuestion, + pendingPlanApproval, onFocus, onSend, onCancel, onRespondPermission, onAnswerQuestion, onAnswerAskQuestion, + onAnswerPlanApproval, children, modes, configOptions, @@ -219,6 +230,17 @@ export function ConversationShell({ />
)} + {pendingPlanApproval && ( +
+ {/* key on approval_id so the card always remounts (fresh in-flight / + feedback state) if the slot is ever reused for a new approval. */} + +
+ )} {!hideInput && feedbackList && (
{feedbackList}
diff --git a/src/components/chat/plan-approval-card.test.tsx b/src/components/chat/plan-approval-card.test.tsx new file mode 100644 index 000000000..085c1904a --- /dev/null +++ b/src/components/chat/plan-approval-card.test.tsx @@ -0,0 +1,106 @@ +import { fireEvent, render, screen, waitFor } from "@testing-library/react" +import { NextIntlClientProvider } from "next-intl" +import { describe, expect, it, vi } from "vitest" + +// Exercise the real Streamdown pipeline for the plan markdown; only the +// link-safety hook is stubbed (no bearing on plan rendering), mirroring +// plan-mode-card.test.tsx. +vi.mock("@/components/ai-elements/link-safety", () => ({ + useStreamdownLinkSafety: () => ({ enabled: false }), +})) + +import { PlanApprovalCard } from "./plan-approval-card" +import enMessages from "@/i18n/messages/en.json" +import type { PendingPlanApprovalState, PlanApprovalAnswer } from "@/lib/types" + +function make(plan: string): PendingPlanApprovalState { + return { + approval_id: "ap-1", + tool_call_id: "call-1", + plan_markdown: plan, + created_at: "2026-01-01T00:00:00Z", + } +} + +function renderCard( + approval: PendingPlanApprovalState, + onAnswer: ( + approvalId: string, + answer: PlanApprovalAnswer + ) => void | Promise = vi.fn() +) { + render( + + + + ) + return onAnswer +} + +describe("PlanApprovalCard", () => { + it("renders the plan markdown and the three actions", async () => { + renderCard(make("# Migration plan\n\n- step one")) + await waitFor(() => + expect(screen.getByText("Migration plan")).toBeInTheDocument() + ) + expect( + screen.getByRole("button", { name: "Approve & build" }) + ).toBeInTheDocument() + expect( + screen.getByRole("button", { name: "Request changes" }) + ).toBeInTheDocument() + expect(screen.getByRole("button", { name: "Abandon" })).toBeInTheDocument() + }) + + it("approves with an approve decision", () => { + const onAnswer = renderCard(make("plan")) + fireEvent.click(screen.getByRole("button", { name: "Approve & build" })) + expect(onAnswer).toHaveBeenCalledWith("ap-1", { + decision: "approve", + feedback: null, + }) + }) + + it("abandons with an abandon decision", () => { + const onAnswer = renderCard(make("plan")) + fireEvent.click(screen.getByRole("button", { name: "Abandon" })) + expect(onAnswer).toHaveBeenCalledWith("ap-1", { + decision: "abandon", + feedback: null, + }) + }) + + it("request-changes reveals a textarea and submits the trimmed feedback", () => { + const onAnswer = renderCard(make("plan")) + // Not requested yet: no feedback textarea. + expect(screen.queryByPlaceholderText("What should change?")).toBeNull() + + fireEvent.click(screen.getByRole("button", { name: "Request changes" })) + const textarea = screen.getByPlaceholderText("What should change?") + + // Send stays disabled until non-empty feedback is typed. + const send = screen.getByRole("button", { name: "Send" }) + expect(send).toBeDisabled() + + fireEvent.change(textarea, { target: { value: " use SSE " } }) + expect(send).not.toBeDisabled() + fireEvent.click(send) + expect(onAnswer).toHaveBeenCalledWith("ap-1", { + decision: "request_changes", + feedback: "use SSE", + }) + }) + + it("shows the empty-plan notice when no plan was written", () => { + renderCard(make(" ")) + expect( + screen.getByText( + "The agent didn't write a plan. Approve to start building, or request changes." + ) + ).toBeInTheDocument() + // Actions still available so the user can approve or push back. + expect( + screen.getByRole("button", { name: "Approve & build" }) + ).toBeInTheDocument() + }) +}) diff --git a/src/components/chat/plan-approval-card.tsx b/src/components/chat/plan-approval-card.tsx new file mode 100644 index 000000000..ba3bc3f4d --- /dev/null +++ b/src/components/chat/plan-approval-card.tsx @@ -0,0 +1,146 @@ +"use client" + +import { useRef, useState } from "react" +import { useTranslations } from "next-intl" +import { ListTodoIcon, Loader2 } from "lucide-react" + +import { Button } from "@/components/ui/button" +import { Textarea } from "@/components/ui/textarea" +import { MessageResponse } from "@/components/ai-elements/message" +import type { + PendingPlanApprovalState, + PlanApprovalAnswer, + PlanApprovalDecision, +} from "@/lib/types" + +interface PlanApprovalCardProps { + /** The awaiting-decision plan approval. The shell renders this card only when + * an approval is pending, so the prop is always present. */ + approval: PendingPlanApprovalState + /** Resolves the parked `exit_plan_mode` ext request. Returns a promise so the + * card can hold an in-flight state and surface a retryable error on failure. */ + onAnswer: ( + approvalId: string, + answer: PlanApprovalAnswer + ) => void | Promise +} + +/** + * Grok plan-mode approval card. When the agent finishes planning it calls + * `exit_plan_mode` and BLOCKS on the user's decision; this renders the plan above + * the composer with Approve / Request-changes / Abandon actions (mirroring Grok's + * own TUI approval bar). "Request changes" reveals a textarea for freeform + * revision notes. Modeled on `AskQuestionCard`'s in-flight/disable pattern: on + * success the backend's `plan_approval_resolved` clears `pendingPlanApproval` and + * unmounts this card, so controls stay disabled rather than flashing back on. + */ +export function PlanApprovalCard({ + approval, + onAnswer, +}: PlanApprovalCardProps) { + const t = useTranslations("Folder.chat.planApproval") + const [submitting, setSubmitting] = useState(false) + const [error, setError] = useState(false) + const [changesOpen, setChangesOpen] = useState(false) + const [feedback, setFeedback] = useState("") + const inFlight = useRef(false) + + const plan = approval.plan_markdown.trim() + + // Run a decision round-trip, holding the card in an in-flight state until it + // resolves. On failure re-enable and surface a retryable error. + const run = async (answer: PlanApprovalAnswer) => { + if (inFlight.current) return + inFlight.current = true + setSubmitting(true) + setError(false) + try { + await onAnswer(approval.approval_id, answer) + inFlight.current = false + } catch { + setError(true) + setSubmitting(false) + inFlight.current = false + } + } + + const decide = (decision: PlanApprovalDecision, fb?: string) => + void run({ decision, feedback: fb ?? null }) + + const locked = submitting + + return ( +
+
+ + {t("title")} +
+ + {plan ? ( +
+ {plan} +
+ ) : ( +

+ {t("emptyPlan")} +

+ )} + + {changesOpen ? ( +
+