From dd3f1dc659e20094f3038f77cbf58cacdb4641f6 Mon Sep 17 00:00:00 2001 From: Harry19081 <20519290+Harry19081@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:48:51 +0800 Subject: [PATCH] refactor(cli): replace arg-count suppressions with request structs PR #565 papered over two `too_many_arguments` hits in the CLI run commands with `#[allow]`. Group the arguments into `CliRunRequest` / `CliMessageRequest` instead, matching the `request: T` command convention already used by `benchmark_*`, `human_session_*`, and pagination. `turn_intent_id` + `client_message_id` move into a `TurnIdentity` pair so the "adopt the client's ids or mint fresh ones" rule lives in one place rather than being duplicated across the two entry points. --- .../agent_sessions/cli/agent_core_bridge.rs | 40 ++--- .../src/agent_sessions/cli/commands/run.rs | 150 ++++++++++++------ src-tauri/src/api/agent/test/cli.rs | 120 ++++++-------- src/api/tauri/rpc/schemas/cli.ts | 8 +- .../cli/__tests__/cliTransport.test.ts | 10 +- .../sync/adapters/cli/cliTransport.ts | 24 +-- 6 files changed, 193 insertions(+), 159 deletions(-) diff --git a/src-tauri/src/agent_sessions/cli/agent_core_bridge.rs b/src-tauri/src/agent_sessions/cli/agent_core_bridge.rs index 4dfc6a423..4dc612d7d 100644 --- a/src-tauri/src/agent_sessions/cli/agent_core_bridge.rs +++ b/src-tauri/src/agent_sessions/cli/agent_core_bridge.rs @@ -17,7 +17,10 @@ use agent_core::interaction::plan_approval::{self, PlanResolution}; use agent_core::session::AgentExecMode; use agent_core::tools::names as tool_names; -use super::commands::{cli_agent_create, cli_agent_delete, cli_agent_message, cli_agent_run}; +use super::commands::{ + cli_agent_create, cli_agent_delete, cli_agent_message, cli_agent_run, CliMessageRequest, + CliRunRequest, +}; use super::persistence::{self, CreateCodeSessionParams}; fn run( @@ -59,14 +62,14 @@ fn run( let created_at = session.created_at.clone(); if !params.user_input.trim().is_empty() { - if let Err(err) = cli_agent_run( - session_id.clone(), - params.user_input, - None, - params.ide_context, - params.mode, - params.images, - ) + if let Err(err) = cli_agent_run(CliRunRequest { + session_id: session_id.clone(), + user_input: params.user_input, + ide_context: params.ide_context, + mode: params.mode, + images: params.images, + ..Default::default() + }) .await { tracing::warn!( @@ -193,17 +196,14 @@ fn respond_plan_approval( edited_marker = if edited { " (edited)" } else { "" }, ); - cli_agent_message( - params.session_id, - synthetic_content, - params.model, - params.account_id, - None, - Some(AgentExecMode::Build.as_str().to_string()), - None, - None, - None, - ) + cli_agent_message(CliMessageRequest { + session_id: params.session_id, + content: synthetic_content, + model: params.model, + account_id: params.account_id, + mode: Some(AgentExecMode::Build.as_str().to_string()), + ..Default::default() + }) .await .map(|_| ()) }) diff --git a/src-tauri/src/agent_sessions/cli/commands/run.rs b/src-tauri/src/agent_sessions/cli/commands/run.rs index 5b5113561..316f9746d 100644 --- a/src-tauri/src/agent_sessions/cli/commands/run.rs +++ b/src-tauri/src/agent_sessions/cli/commands/run.rs @@ -6,7 +6,7 @@ use super::super::persistence; use super::super::session_runner; use super::super::types::{KeySource, SessionStatus}; use agent_core::session::IdeContext; -use serde::Serialize; +use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] @@ -16,6 +16,68 @@ pub struct CliRunReceipt { pub status: SessionStatus, } +/// Start one CLI turn on an existing session row. +/// +/// `Default` is derived so callers that only drive a plain prompt (the +/// agent-core bridge, the debug runtime probes) can name just the fields +/// they mean instead of padding the call with positional `None`s. +#[derive(Debug, Default, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct CliRunRequest { + pub session_id: String, + pub user_input: String, + pub cli_resume_id: Option, + pub ide_context: Option, + pub mode: Option, + pub images: Option>, +} + +/// Send a follow-up message on an existing session, optionally switching the +/// model/account first. `turn_intent_id` / `client_message_id` are optional: +/// the frontend pre-assigns them so its optimistic user row and the persisted +/// intent share one identity, while callers without a UI row omit them. +#[derive(Debug, Default, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct CliMessageRequest { + pub session_id: String, + pub content: String, + pub model: Option, + pub account_id: Option, + pub ide_context: Option, + pub mode: Option, + pub images: Option>, + pub turn_intent_id: Option, + pub client_message_id: Option, +} + +/// Identity of a single turn. `turn_intent_id` keys the `turn_intents` row and +/// every `status_changed` broadcast for the turn; `client_message_id` +/// reconciles the frontend's optimistic user message with the persisted one. +#[derive(Debug)] +struct TurnIdentity { + turn_intent_id: String, + client_message_id: String, +} + +impl TurnIdentity { + /// Mint a fresh pair for a turn no client pre-assigned ids for. + fn generate() -> Self { + Self::from_client(None, None) + } + + /// Adopt whichever halves the client supplied, minting the rest. + fn from_client(turn_intent_id: Option, client_message_id: Option) -> Self { + Self { + turn_intent_id: turn_intent_id.unwrap_or_else(new_id), + client_message_id: client_message_id.unwrap_or_else(new_id), + } + } +} + +fn new_id() -> String { + uuid::Uuid::new_v4().to_string() +} + /// Prepend IDE context (open files, git status, etc.) to the user prompt /// so external CLI agents are aware of the user's IDE state. fn inject_ide_context_into_prompt(user_input: &str, ide_context: Option<&IdeContext>) -> String { @@ -45,38 +107,27 @@ pub async fn cli_agent_tui_release(session_id: String) -> Result { /// Run a code session (spawn CLI agent in background). #[tauri::command] -pub async fn cli_agent_run( - session_id: String, - user_input: String, - cli_resume_id: Option, - ide_context: Option, - mode: Option, - images: Option>, -) -> Result<(), String> { - cli_agent_run_internal( +pub async fn cli_agent_run(request: CliRunRequest) -> Result<(), String> { + run_turn(request, TurnIdentity::generate()).await +} + +/// Shared turn body behind both `cli_agent_run` and `cli_agent_message`: +/// persist acceptance under the registry lock, broadcast `running`, then spawn +/// the background runner. +async fn run_turn(request: CliRunRequest, turn: TurnIdentity) -> Result<(), String> { + let CliRunRequest { session_id, user_input, cli_resume_id, ide_context, mode, images, - uuid::Uuid::new_v4().to_string(), - uuid::Uuid::new_v4().to_string(), - ) - .await -} + } = request; + let TurnIdentity { + turn_intent_id, + client_message_id, + } = turn; -#[allow(clippy::too_many_arguments)] -async fn cli_agent_run_internal( - session_id: String, - user_input: String, - cli_resume_id: Option, - ide_context: Option, - mode: Option, - images: Option>, - turn_intent_id: String, - client_message_id: String, -) -> Result<(), String> { tracing::info!( session_id = %session_id, has_resume_id = cli_resume_id.is_some(), @@ -213,20 +264,19 @@ async fn cli_agent_run_internal( /// If `model` or `account_id` is provided, updates the session config before /// re-running so the CLI uses the newly selected model/key. #[tauri::command] -#[allow(clippy::too_many_arguments)] -pub async fn cli_agent_message( - session_id: String, - content: String, - model: Option, - account_id: Option, - ide_context: Option, - mode: Option, - images: Option>, - turn_intent_id: Option, - client_message_id: Option, -) -> Result { - let turn_intent_id = turn_intent_id.unwrap_or_else(|| uuid::Uuid::new_v4().to_string()); - let client_message_id = client_message_id.unwrap_or_else(|| uuid::Uuid::new_v4().to_string()); +pub async fn cli_agent_message(request: CliMessageRequest) -> Result { + let CliMessageRequest { + session_id, + content, + model, + account_id, + ide_context, + mode, + images, + turn_intent_id, + client_message_id, + } = request; + let turn = TurnIdentity::from_client(turn_intent_id, client_message_id); tracing::info!( session_id = %session_id, has_model_override = model.is_some(), @@ -377,15 +427,17 @@ pub async fn cli_agent_message( // Re-run the session with the new message tracing::info!(session_id = %session_id, "cli_agent_message: dispatching rerun"); - cli_agent_run_internal( - session_id.clone(), - content, - cli_resume_id, - ide_context, - mode, - images, - turn_intent_id.clone(), - client_message_id, + let turn_intent_id = turn.turn_intent_id.clone(); + run_turn( + CliRunRequest { + session_id: session_id.clone(), + user_input: content, + cli_resume_id, + ide_context, + mode, + images, + }, + turn, ) .await?; Ok(CliRunReceipt { diff --git a/src-tauri/src/api/agent/test/cli.rs b/src-tauri/src/api/agent/test/cli.rs index 6bd868984..9fc15e91c 100644 --- a/src-tauri/src/api/agent/test/cli.rs +++ b/src-tauri/src/api/agent/test/cli.rs @@ -10,7 +10,7 @@ use tokio::process::Command; use crate::agent_sessions::cli::commands::{ cli_agent_chunks, cli_agent_create, cli_agent_message, cli_agent_resume, cli_agent_run, - cli_agent_status, + cli_agent_status, CliMessageRequest, CliRunRequest, }; use crate::agent_sessions::cli::persistence::{self, CreateCodeSessionParams}; use crate::agent_sessions::cli::session_runner; @@ -196,14 +196,11 @@ pub async fn test_cursor_cli_runtime( })); } - if let Err(err) = cli_agent_run( - session_id.clone(), - request.content.clone(), - None, - None, - None, - None, - ) + if let Err(err) = cli_agent_run(CliRunRequest { + session_id: session_id.clone(), + user_input: request.content.clone(), + ..Default::default() + }) .await { return Json(json!({ "error": format!("cli_agent_run failed: {err}") })); @@ -301,14 +298,11 @@ pub async fn test_cursor_cli_account_switch( Err(err) => return Json(json!({ "error": format!("cli_agent_create failed: {err}") })), }; let session_id = created.session_id; - if let Err(err) = cli_agent_run( - session_id.clone(), - request.initial_content.clone(), - None, - None, - None, - None, - ) + if let Err(err) = cli_agent_run(CliRunRequest { + session_id: session_id.clone(), + user_input: request.initial_content.clone(), + ..Default::default() + }) .await { return Json(json!({ "error": format!("initial cli_agent_run failed: {err}") })); @@ -320,17 +314,12 @@ pub async fn test_cursor_cli_account_switch( Err(err) => return Json(json!({ "error": err, "session_id": session_id })), }; - if let Err(err) = cli_agent_message( - session_id.clone(), - request.followup_content.clone(), - None, - Some(request.followup_account_id.clone()), - None, - None, - None, - None, - None, - ) + if let Err(err) = cli_agent_message(CliMessageRequest { + session_id: session_id.clone(), + content: request.followup_content.clone(), + account_id: Some(request.followup_account_id.clone()), + ..Default::default() + }) .await { return Json(json!({ "error": format!("cli_agent_message failed: {err}") })); @@ -419,14 +408,11 @@ pub async fn test_claude_code_cli_account_switch( Err(err) => return Json(json!({ "error": format!("cli_agent_create failed: {err}") })), }; let session_id = created.session_id; - if let Err(err) = cli_agent_run( - session_id.clone(), - request.initial_content.clone(), - None, - None, - None, - None, - ) + if let Err(err) = cli_agent_run(CliRunRequest { + session_id: session_id.clone(), + user_input: request.initial_content.clone(), + ..Default::default() + }) .await { return Json(json!({ "error": format!("initial cli_agent_run failed: {err}") })); @@ -444,17 +430,13 @@ pub async fn test_claude_code_cli_account_switch( let baseline_chunk_count = initial_chunks.len(); let baseline_updated_at = initial_session.updated_at.clone(); - if let Err(err) = cli_agent_message( - session_id.clone(), - request.followup_content.clone(), - Some(model.clone()), - Some(request.followup_account_id.clone()), - None, - None, - None, - None, - None, - ) + if let Err(err) = cli_agent_message(CliMessageRequest { + session_id: session_id.clone(), + content: request.followup_content.clone(), + model: Some(model.clone()), + account_id: Some(request.followup_account_id.clone()), + ..Default::default() + }) .await { return Json(json!({ "error": format!("cli_agent_message failed: {err}") })); @@ -580,14 +562,11 @@ pub async fn test_codex_cli_account_switch( let initial_codex_home = app_paths::codex_cli_profile_dir(&request.initial_account_id); let followup_codex_home = app_paths::codex_cli_profile_dir(&request.followup_account_id); - if let Err(err) = cli_agent_run( - session_id.clone(), - request.initial_content.clone(), - None, - None, - None, - None, - ) + if let Err(err) = cli_agent_run(CliRunRequest { + session_id: session_id.clone(), + user_input: request.initial_content.clone(), + ..Default::default() + }) .await { return Json(json!({ "error": format!("initial cli_agent_run failed: {err}") })); @@ -605,17 +584,13 @@ pub async fn test_codex_cli_account_switch( let baseline_chunk_count = initial_chunks.len(); let baseline_updated_at = initial_session.updated_at.clone(); - if let Err(err) = cli_agent_message( - session_id.clone(), - request.followup_content.clone(), - Some(model.clone()), - Some(request.followup_account_id.clone()), - None, - None, - None, - None, - None, - ) + if let Err(err) = cli_agent_message(CliMessageRequest { + session_id: session_id.clone(), + content: request.followup_content.clone(), + model: Some(model.clone()), + account_id: Some(request.followup_account_id.clone()), + ..Default::default() + }) .await { return Json(json!({ "error": format!("cli_agent_message failed: {err}") })); @@ -798,14 +773,11 @@ pub async fn test_cli_resume_lock_isolation() -> Json { tokio::time::sleep(std::time::Duration::from_millis(150)).await; let peer_start = std::time::Instant::now(); - let peer_result = cli_agent_run( - peer_session_id.clone(), - "E2E peer start should not wait on unrelated resume cleanup".to_string(), - None, - None, - None, - None, - ) + let peer_result = cli_agent_run(CliRunRequest { + session_id: peer_session_id.clone(), + user_input: "E2E peer start should not wait on unrelated resume cleanup".to_string(), + ..Default::default() + }) .await; let peer_start_ms = peer_start.elapsed().as_millis() as u64; diff --git a/src/api/tauri/rpc/schemas/cli.ts b/src/api/tauri/rpc/schemas/cli.ts index 96e91b70b..44866bd7f 100644 --- a/src/api/tauri/rpc/schemas/cli.ts +++ b/src/api/tauri/rpc/schemas/cli.ts @@ -2,7 +2,7 @@ import { z } from "zod/v4"; import { ActivityChunkSchema } from "@src/api/realtime/websocket/schemas"; -export const CliMessageInputSchema = z.object({ +export const CliMessageRequestSchema = z.object({ sessionId: z.string().min(1), content: z.string(), turnIntentId: z.string().min(1), @@ -14,6 +14,12 @@ export const CliMessageInputSchema = z.object({ images: z.array(z.string()).optional(), }); +/** `cli_agent_message` takes a single `request` struct, like the other + * multi-field commands (`benchmark_*`, `human_session_*`, pagination). */ +export const CliMessageInputSchema = z.object({ + request: CliMessageRequestSchema, +}); + export const CliRunReceiptSchema = z.object({ sessionId: z.string(), turnIntentId: z.string(), diff --git a/src/engines/SessionCore/sync/adapters/cli/__tests__/cliTransport.test.ts b/src/engines/SessionCore/sync/adapters/cli/__tests__/cliTransport.test.ts index 5b6438444..4a44fd031 100644 --- a/src/engines/SessionCore/sync/adapters/cli/__tests__/cliTransport.test.ts +++ b/src/engines/SessionCore/sync/adapters/cli/__tests__/cliTransport.test.ts @@ -41,10 +41,12 @@ describe("sendCliMessage acceptance boundary", () => { ).resolves.toBeUndefined(); expect(mocks.message).toHaveBeenCalledWith({ - sessionId: "cliagent-worker", - content: "continue", - turnIntentId: "intent-1", - clientMessageId: "message-1", + request: { + sessionId: "cliagent-worker", + content: "continue", + turnIntentId: "intent-1", + clientMessageId: "message-1", + }, }); expect(mocks.registerReceipt).toHaveBeenCalledWith({ sessionId: "cliagent-worker", diff --git a/src/engines/SessionCore/sync/adapters/cli/cliTransport.ts b/src/engines/SessionCore/sync/adapters/cli/cliTransport.ts index b19d70e49..7b2417948 100644 --- a/src/engines/SessionCore/sync/adapters/cli/cliTransport.ts +++ b/src/engines/SessionCore/sync/adapters/cli/cliTransport.ts @@ -26,17 +26,19 @@ export async function sendCliMessage(input: AdapterSendInput): Promise { const turnIntentId = input.turnIntentId ?? newMessageId(); const clientMessageId = input.clientMessageId ?? newMessageId(); const receipt = await rpc.cli.message({ - sessionId, - content, - turnIntentId, - clientMessageId, - ...(model ? { model } : {}), - ...(accountId ? { accountId } : {}), - ...(mode ? { mode } : {}), - ...(imageDataUrls && imageDataUrls.length > 0 - ? { images: imageDataUrls } - : {}), - ...(adeContext ? { ideContext: adeContext } : {}), + request: { + sessionId, + content, + turnIntentId, + clientMessageId, + ...(model ? { model } : {}), + ...(accountId ? { accountId } : {}), + ...(mode ? { mode } : {}), + ...(imageDataUrls && imageDataUrls.length > 0 + ? { images: imageDataUrls } + : {}), + ...(adeContext ? { ideContext: adeContext } : {}), + }, }); cliTurnLifecycleCoordinator.registerReceipt(receipt); }