From aa849d1bb2d275af3a42536a0f71964a60369f03 Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Wed, 15 Jul 2026 22:53:00 +0800 Subject: [PATCH 1/5] fix: surface compaction lifecycle and stabilize provider retries --- .../devo-ai-sdk/src/v2/client.test.ts | 94 +++++ .../packages/devo-ai-sdk/src/v2/client.ts | 52 ++- .../components/chat/chat-turn.test.ts | 2 +- .../chat/compaction-status-divider.tsx | 2 +- crates/core/models.json | 4 +- crates/core/src/query.rs | 378 +++++++++++++++++- crates/core/tests/context_limit_compaction.rs | 152 +++++++ crates/provider/src/error.rs | 27 ++ crates/provider/src/http.rs | 20 + crates/provider/src/openai/error_payload.rs | 48 +++ .../tests/openai_chat_completions_stream.rs | 49 ++- crates/server/src/runtime/connection.rs | 121 +++++- crates/server/src/runtime/outbound.rs | 126 +++++- .../runtime/turn_exec/context_compaction.rs | 167 ++++++++ .../src/runtime/turn_exec/event_stream.rs | 37 +- crates/server/src/runtime/turn_exec/mod.rs | 1 + crates/server/src/runtime/turn_exec/tests.rs | 171 +++++++- crates/server/src/runtime/turn_exec/trace.rs | 55 ++- crates/tui/src/chatwidget/text_stream.rs | 23 ++ crates/tui/src/chatwidget/transcript_view.rs | 36 +- crates/tui/src/chatwidget/worker_events.rs | 95 +++-- crates/tui/src/chatwidget_tests.rs | 221 +++++++++- crates/tui/src/events.rs | 4 +- crates/tui/src/exec_cell/model.rs | 4 + crates/tui/src/worker.rs | 167 +++++--- .../tui/src/worker_queue_compaction_tests.rs | 2 +- 26 files changed, 1893 insertions(+), 165 deletions(-) create mode 100644 crates/core/tests/context_limit_compaction.rs create mode 100644 crates/server/src/runtime/turn_exec/context_compaction.rs diff --git a/apps/desktop/packages/devo-ai-sdk/src/v2/client.test.ts b/apps/desktop/packages/devo-ai-sdk/src/v2/client.test.ts index 3340664b..bc0e6ebe 100644 --- a/apps/desktop/packages/devo-ai-sdk/src/v2/client.test.ts +++ b/apps/desktop/packages/devo-ai-sdk/src/v2/client.test.ts @@ -1844,6 +1844,100 @@ describe("ACP desktop SDK session mapping", () => { }) }) + test("bridges turn context-compaction items into lifecycle events and a transcript marker", async () => { + Date.now = () => 1_772_000_000_000 + const transport = new FakeTransport((method) => { + if (method === "initialize") return initializeResult + if (method === "session/list") return { sessions: [sessionInfo] } + throw new Error(`unexpected request ${method}`) + }) + const client = createDevoClient({ directory: "/repo", transport }) + const stream = (await client.global.event()).stream[Symbol.asyncIterator]() + + await client.session.list() + const context = { + session_id: "s1", + turn_id: "turn-1", + item_id: "compact-1", + seq: 9, + } + const item = { + item_id: "compact-1", + item_kind: "context_compaction", + payload: { title: "Compaction started" }, + } + transport.emitSessionUpdate({ + sessionId: "s1", + update: { sessionUpdate: "session_info_update" }, + _meta: { + "devo/originalMethod": "item/started", + "devo/originalEvent": { kind: "item_started", context, item }, + }, + } satisfies AcpSessionNotification) + + expect(await nextPayloadOfType(stream, "session.compaction.started", "item started")).toEqual({ + type: "session.compaction.started", + properties: { sessionID: "s1" }, + }) + expect(await nextPayloadOfType(stream, "message.part.updated", "compaction marker")).toEqual({ + type: "message.part.updated", + properties: { + part: { + id: "compaction-compact-1-text", + sessionID: "s1", + messageID: "compaction-compact-1", + type: "text", + text: "Session compaction started.", + time: { start: 1_772_000_000_000 }, + }, + }, + }) + + transport.emitSessionUpdate({ + sessionId: "s1", + update: { sessionUpdate: "session_info_update" }, + _meta: { + "devo/originalMethod": "item/completed", + "devo/originalEvent": { + kind: "item_completed", + context, + item: { ...item, payload: { title: "Context compacted" } }, + }, + }, + } satisfies AcpSessionNotification) + + expect(await nextPayloadOfType(stream, "session.compaction.completed", "item completed")).toEqual({ + type: "session.compaction.completed", + properties: { sessionID: "s1" }, + }) + + transport.emitSessionUpdate({ + sessionId: "s1", + update: { sessionUpdate: "session_info_update" }, + _meta: { + "devo/originalMethod": "item/completed", + "devo/originalEvent": { + kind: "item_completed", + context: { ...context, item_id: "compact-2" }, + item: { + ...item, + item_id: "compact-2", + payload: { + title: "Compaction failed", + status: "failed", + message: "summary unavailable", + }, + }, + }, + }, + } satisfies AcpSessionNotification) + + expect(await nextPayloadOfType(stream, "session.compaction.failed", "item failed")).toEqual({ + type: "session.compaction.failed", + properties: { sessionID: "s1", message: "summary unavailable" }, + }) + }) + test("refreshes session status from session info update metadata", async () => { const transport = new FakeTransport((method) => { if (method === "initialize") return initializeResult diff --git a/apps/desktop/packages/devo-ai-sdk/src/v2/client.ts b/apps/desktop/packages/devo-ai-sdk/src/v2/client.ts index d128ee9f..e2b9ead8 100644 --- a/apps/desktop/packages/devo-ai-sdk/src/v2/client.ts +++ b/apps/desktop/packages/devo-ai-sdk/src/v2/client.ts @@ -387,6 +387,9 @@ function sessionStatusChangedFromOriginalEvent( function sessionIdFromCompactionPayload(payload: Record): string | null { const direct = payload.session_id ?? payload.sessionId if (typeof direct === "string" && direct) return direct + const context = objectRecord(payload.context) + const contextual = context?.session_id ?? context?.sessionId + if (typeof contextual === "string" && contextual) return contextual const session = objectRecord(payload.session) const nested = session?.session_id ?? session?.sessionId return typeof nested === "string" && nested ? nested : null @@ -395,13 +398,38 @@ function sessionIdFromCompactionPayload(payload: Record): strin function sessionCompactionFromOriginalEvent( original: unknown, originalMethod?: string, -): { sessionId: string; status: "started" | "completed" | "failed"; message?: string } | null { +): { + sessionId: string + status: "started" | "completed" | "failed" + message?: string + itemId?: string + turnId?: string +} | null { const event = objectRecord(original) if (!event) return null let status: "started" | "completed" | "failed" | null = null let payload: Record | undefined - if (originalMethod === "session/compaction/started") { + let itemId: string | undefined + let turnId: string | undefined + if (originalMethod === "item/started" || originalMethod === "item/completed") { + const item = objectRecord(event.item) + if (item?.item_kind !== "context_compaction" && item?.itemKind !== "context_compaction") { + return null + } + const context = objectRecord(event.context) + const itemPayload = objectRecord(item.payload) + status = originalMethod === "item/started" + ? "started" + : itemPayload?.status === "failed" + ? "failed" + : "completed" + payload = event + const rawItemId = item.item_id ?? item.itemId + const rawTurnId = context?.turn_id ?? context?.turnId + itemId = typeof rawItemId === "string" && rawItemId ? rawItemId : undefined + turnId = typeof rawTurnId === "string" && rawTurnId ? rawTurnId : undefined + } else if (originalMethod === "session/compaction/started") { status = "started" payload = objectRecord(event.SessionCompactionStarted) ?? event } else if (originalMethod === "session/compaction/completed") { @@ -443,11 +471,14 @@ function sessionCompactionFromOriginalEvent( if (!status || !payload) return null const sessionId = sessionIdFromCompactionPayload(payload) if (!sessionId) return null - const message = payload.message + const itemPayload = objectRecord(objectRecord(payload.item)?.payload) + const message = payload.message ?? itemPayload?.message return { sessionId, status, ...(typeof message === "string" && message ? { message } : {}), + ...(itemId ? { itemId } : {}), + ...(turnId ? { turnId } : {}), } } @@ -1646,6 +1677,21 @@ class AcpClient { ...(compaction.message ? { message: compaction.message } : {}), }, }) + if (compaction.status === "started" && compaction.itemId) { + const messageId = `compaction-${compaction.itemId}` + const markerExists = this.parts + .get(partCacheKey(sessionId, messageId)) + ?.some((part) => part.type === "text" && part.text === "Session compaction started.") + if (!markerExists) { + this.appendText(sessionId, directory, "assistant", "text", { + content: { text: "Session compaction started." }, + messageId, + ...(compaction.turnId + ? { _meta: { [DEVO_TURN_ID_META]: compaction.turnId } } + : {}), + }) + } + } return } const payload = requestUserInputFromOriginalEvent(original) diff --git a/apps/desktop/src/renderer/components/chat/chat-turn.test.ts b/apps/desktop/src/renderer/components/chat/chat-turn.test.ts index 73867f0e..0cb61cad 100644 --- a/apps/desktop/src/renderer/components/chat/chat-turn.test.ts +++ b/apps/desktop/src/renderer/components/chat/chat-turn.test.ts @@ -291,7 +291,7 @@ describe("ChatTurnComponent transcript controls", () => { compactionDividerSource.includes("BubblesIcon") && compactionDividerSource.includes("PackageCheckIcon"), usesRequestedLabels: - compactionDividerSource.includes("Compacting context") && + compactionDividerSource.includes("Compaction started") && compactionDividerSource.includes("Context compacted"), keepsIconStyleConsistent: compactionDividerSource.includes("size-3.5") && diff --git a/apps/desktop/src/renderer/components/chat/compaction-status-divider.tsx b/apps/desktop/src/renderer/components/chat/compaction-status-divider.tsx index 01cbaa55..d4b83751 100644 --- a/apps/desktop/src/renderer/components/chat/compaction-status-divider.tsx +++ b/apps/desktop/src/renderer/components/chat/compaction-status-divider.tsx @@ -17,7 +17,7 @@ export function CompactionStatusDivider({ }) { const isCompleted = status === "completed"; const Icon = isCompleted ? PackageCheckIcon : BubblesIcon; - const label = isCompleted ? "Context compacted" : "Compacting context"; + const label = isCompleted ? "Context compacted" : "Compaction started"; return (
usize { pub enum QueryEvent { /// Provider request retry status. ProviderRetryStatus(ProviderRetryStatus), + /// Context compaction is about to begin. + ContextCompactionStarted, + /// Context compaction replaced the current prompt history. + ContextCompactionCompleted, + /// Context compaction did not replace the current prompt history. + ContextCompactionFailed { + /// Human-readable reason the compaction did not complete. + message: String, + }, /// Incremental text from the assistant. TextDelta(String), /// Incremental reasoning text from the assistant. @@ -429,7 +438,15 @@ fn provider_retry_decision( ProviderRetryDecision::CompactAndRetry } } - ErrorClass::RateLimit | ErrorClass::ServerError | ErrorClass::NetworkError => { + ErrorClass::RateLimit => { + if *retry_count >= MAX_RETRIES { + ProviderRetryDecision::Fail + } else { + *retry_count += 1; + ProviderRetryDecision::RetryAfter(RATE_LIMIT_RETRY_DELAY) + } + } + ErrorClass::ServerError | ErrorClass::NetworkError => { if *retry_count >= MAX_RETRIES { ProviderRetryDecision::Fail } else { @@ -461,6 +478,7 @@ fn provider_retry_decision( /// `context_too_long`; keeps from the latest user message onward. async fn summarize_and_compact( session: &mut SessionState, + on_event: &Option, provider: &Arc, model_slug: &str, request_model: &str, @@ -492,6 +510,7 @@ async fn summarize_and_compact( max_tokens, ); + emit_query_event(on_event, QueryEvent::ContextCompactionStarted).await; match compact_history(&items, &token_info, &summarizer, &config).await { Ok(CompactAction::Replaced(compacted_items)) => { let new_messages: Vec = compacted_items @@ -507,12 +526,27 @@ async fn summarize_and_compact( .saturating_sub(new_messages.len()); info!("LLM compaction removed {removed} messages"); session.set_prompt_messages(new_messages); + emit_query_event(on_event, QueryEvent::ContextCompactionCompleted).await; } Ok(CompactAction::Skipped) => { debug!("LLM compaction skipped, nothing to compact"); + emit_query_event( + on_event, + QueryEvent::ContextCompactionFailed { + message: "Context compaction skipped: nothing to compact".to_string(), + }, + ) + .await; } Err(e) => { warn!("LLM compaction failed: {e}"); + emit_query_event( + on_event, + QueryEvent::ContextCompactionFailed { + message: e.to_string(), + }, + ) + .await; } } } @@ -813,6 +847,7 @@ fn dsml_text_tool_call_continuation_message( const MAX_RETRIES: usize = 5; const INITIAL_RETRY_BACKOFF_MS: u64 = 250; +const RATE_LIMIT_RETRY_DELAY: Duration = Duration::from_secs(60); /// TODO: The body of `query` is too lengthy, we should move out `stream lop` out, I am /// not sure whether we should do this. @@ -996,6 +1031,7 @@ pub async fn query_with_options( // Example: [user1, asst1, user2, asst2, user3] -> [summary, asst2, user3]. summarize_and_compact( session, + &on_event, &provider, &compaction_model_slug, &compaction_request_model, @@ -1134,6 +1170,7 @@ pub async fn query_with_options( // with the provider; preserve from latest user only. summarize_and_compact( session, + &on_event, &provider, &compaction_model_slug, &compaction_request_model, @@ -1324,6 +1361,7 @@ pub async fn query_with_options( // with the provider; preserve from latest user only. summarize_and_compact( session, + &on_event, &provider, &compaction_model_slug, &compaction_request_model, @@ -1983,6 +2021,10 @@ mod tests { use crate::Model; use crate::ReasoningEffort; use crate::Role; + use crate::context::ContextualUserFragment; + use crate::context::compaction_summary::CompactionSummary; + use crate::history::compaction::CompactionKind; + use crate::response_item::ResponseItem; #[test] fn assistant_content_visibility_requires_visible_content() { @@ -2536,6 +2578,20 @@ mod tests { attempts: AtomicUsize, } + struct RateLimitedStreamCreateProvider { + attempts: AtomicUsize, + } + + enum CompactionProviderOutcome { + Summary, + Error, + } + + struct CompactionProvider { + completion_calls: AtomicUsize, + outcome: CompactionProviderOutcome, + } + #[async_trait] impl devo_provider::ModelProviderSDK for CapturingProvider { async fn completion(&self, _request: ModelRequest) -> Result { @@ -2830,6 +2886,59 @@ mod tests { } } + #[async_trait] + impl devo_provider::ModelProviderSDK for RateLimitedStreamCreateProvider { + async fn completion(&self, _request: ModelRequest) -> Result { + unreachable!("tests stream responses only") + } + + async fn completion_stream( + &self, + _request: ModelRequest, + ) -> Result> + Send>>> { + let attempt = self.attempts.fetch_add(1, Ordering::SeqCst); + if attempt < 2 { + return Err(anyhow::anyhow!("429 rate limit exceeded")); + } + + Ok(final_text_stream("done")) + } + + fn name(&self) -> &str { + "rate-limited-stream-create-provider" + } + } + + #[async_trait] + impl devo_provider::ModelProviderSDK for CompactionProvider { + async fn completion(&self, _request: ModelRequest) -> Result { + self.completion_calls.fetch_add(1, Ordering::SeqCst); + match &self.outcome { + CompactionProviderOutcome::Summary => Ok(ModelResponse { + id: "compaction-response".to_string(), + content: vec![ResponseContent::Text("summary".to_string())], + stop_reason: Some(StopReason::EndTurn), + usage: Usage::default(), + metadata: Default::default(), + }), + CompactionProviderOutcome::Error => { + Err(anyhow::anyhow!("compaction provider failed")) + } + } + } + + async fn completion_stream( + &self, + _request: ModelRequest, + ) -> Result> + Send>>> { + unreachable!("tests call the non-streaming compaction path only") + } + + fn name(&self) -> &str { + "compaction-provider" + } + } + #[async_trait] impl ToolHandler for MutatingTool { fn spec(&self) -> &crate::tools::tool_spec::ToolSpec { @@ -3033,6 +3142,170 @@ mod tests { } } + #[derive(Debug, PartialEq, Eq)] + enum RecordedCompactionEvent { + Started, + Completed, + Failed { message: String }, + } + + fn recorded_compaction_events(events: &[QueryEvent]) -> Vec { + events + .iter() + .filter_map(|event| match event { + QueryEvent::ContextCompactionStarted => Some(RecordedCompactionEvent::Started), + QueryEvent::ContextCompactionCompleted => Some(RecordedCompactionEvent::Completed), + QueryEvent::ContextCompactionFailed { message } => { + Some(RecordedCompactionEvent::Failed { + message: message.clone(), + }) + } + QueryEvent::ProviderRetryStatus(_) + | QueryEvent::TextDelta(_) + | QueryEvent::ReasoningDelta(_) + | QueryEvent::ReasoningCompleted + | QueryEvent::UsageDelta { .. } + | QueryEvent::ToolUseStart { .. } + | QueryEvent::ToolExecutionStart { .. } + | QueryEvent::ToolProgress { .. } + | QueryEvent::ToolResult { .. } + | QueryEvent::TurnComplete { .. } + | QueryEvent::Usage { .. } => None, + }) + .collect() + } + + fn recording_callback(events: &Arc>>) -> EventCallback { + let captured_events = Arc::clone(events); + Arc::new(move |event| { + let captured_events = Arc::clone(&captured_events); + Box::pin(async move { + captured_events.lock().expect("lock events").push(event); + }) + }) + } + + fn compaction_test_session(total_input_tokens: usize) -> SessionState { + let mut session = SessionState::new(SessionConfig::default(), std::env::temp_dir()); + session.push_message(Message::user("x".repeat(80_004))); + session.push_message(Message::user("latest")); + session.total_input_tokens = total_input_tokens; + session + } + + #[tokio::test] + async fn automatic_compaction_emits_started_then_completed_when_history_is_replaced() { + let provider = Arc::new(CompactionProvider { + completion_calls: AtomicUsize::new(0), + outcome: CompactionProviderOutcome::Summary, + }); + let provider_sdk: Arc = provider.clone(); + let events = Arc::new(Mutex::new(Vec::new())); + let on_event = Some(recording_callback(&events)); + let mut session = compaction_test_session(/*total_input_tokens*/ 200_000); + + super::summarize_and_compact( + &mut session, + &on_event, + &provider_sdk, + "compaction-model", + "compaction-request-model", + /*max_tokens*/ 4096, + CompactionKind::Auto, + ) + .await; + + assert_eq!( + recorded_compaction_events(&events.lock().expect("lock events")), + vec![ + RecordedCompactionEvent::Started, + RecordedCompactionEvent::Completed, + ] + ); + assert_eq!(provider.completion_calls.load(Ordering::SeqCst), 1); + let ResponseItem::Message(expected_summary) = + CompactionSummary::new("summary").to_response_item() + else { + unreachable!("compaction summaries are messages"); + }; + assert_eq!( + session.prompt_source_messages(), + &[expected_summary, Message::user("latest")] + ); + } + + #[tokio::test] + async fn automatic_compaction_emits_failed_when_compaction_is_skipped() { + let provider = Arc::new(CompactionProvider { + completion_calls: AtomicUsize::new(0), + outcome: CompactionProviderOutcome::Summary, + }); + let provider_sdk: Arc = provider.clone(); + let events = Arc::new(Mutex::new(Vec::new())); + let on_event = Some(recording_callback(&events)); + let mut session = compaction_test_session(/*total_input_tokens*/ 0); + let original_messages = session.prompt_source_messages().to_vec(); + + super::summarize_and_compact( + &mut session, + &on_event, + &provider_sdk, + "compaction-model", + "compaction-request-model", + /*max_tokens*/ 4096, + CompactionKind::Auto, + ) + .await; + + assert_eq!( + recorded_compaction_events(&events.lock().expect("lock events")), + vec![ + RecordedCompactionEvent::Started, + RecordedCompactionEvent::Failed { + message: "Context compaction skipped: nothing to compact".to_string(), + }, + ] + ); + assert_eq!(provider.completion_calls.load(Ordering::SeqCst), 0); + assert_eq!(session.prompt_source_messages(), original_messages); + } + + #[tokio::test(start_paused = true)] + async fn proactive_compaction_emits_failed_when_compaction_errors() { + let provider = Arc::new(CompactionProvider { + completion_calls: AtomicUsize::new(0), + outcome: CompactionProviderOutcome::Error, + }); + let provider_sdk: Arc = provider.clone(); + let events = Arc::new(Mutex::new(Vec::new())); + let on_event = Some(recording_callback(&events)); + let mut session = compaction_test_session(/*total_input_tokens*/ 0); + let original_messages = session.prompt_source_messages().to_vec(); + + super::summarize_and_compact( + &mut session, + &on_event, + &provider_sdk, + "compaction-model", + "compaction-request-model", + /*max_tokens*/ 4096, + CompactionKind::Proactive, + ) + .await; + + assert_eq!( + recorded_compaction_events(&events.lock().expect("lock events")), + vec![ + RecordedCompactionEvent::Started, + RecordedCompactionEvent::Failed { + message: "summarization failed: compaction provider failed".to_string(), + }, + ] + ); + assert_eq!(provider.completion_calls.load(Ordering::SeqCst), 5); + assert_eq!(session.prompt_source_messages(), original_messages); + } + #[tokio::test] async fn query_retries_transient_stream_creation_errors() { let provider = Arc::new(TransientStreamCreateProvider { @@ -3100,7 +3373,10 @@ mod tests { .iter() .filter_map(|event| match event { QueryEvent::ProviderRetryStatus(status) => Some(status.clone()), - QueryEvent::TextDelta(_) + QueryEvent::ContextCompactionStarted + | QueryEvent::ContextCompactionCompleted + | QueryEvent::ContextCompactionFailed { .. } + | QueryEvent::TextDelta(_) | QueryEvent::ReasoningDelta(_) | QueryEvent::ReasoningCompleted | QueryEvent::UsageDelta { .. } @@ -3143,6 +3419,104 @@ mod tests { assert_eq!(assistant_messages, vec![Message::assistant_text("done")]); } + #[tokio::test(start_paused = true)] + async fn query_waits_sixty_seconds_for_each_rate_limit_retry() { + let provider = Arc::new(RateLimitedStreamCreateProvider { + attempts: AtomicUsize::new(0), + }); + let provider_sdk: Arc = provider.clone(); + let registry = Arc::new(ToolRegistry::new()); + let runtime = ToolRuntime::new_without_permissions(Arc::clone(®istry)); + let mut session = SessionState::new(SessionConfig::default(), std::env::temp_dir()); + session.push_message(Message::user("hello")); + let turn_config = TurnConfig::new(Model::default(), None); + let model = turn_config.model.slug.clone(); + let events = Arc::new(Mutex::new(Vec::new())); + let captured_events = Arc::clone(&events); + let callback: EventCallback = Arc::new(move |event| { + let captured_events = Arc::clone(&captured_events); + Box::pin(async move { + captured_events.lock().expect("lock events").push(event); + }) + }); + let started_at = tokio::time::Instant::now(); + + query( + &mut session, + &turn_config, + provider_sdk, + registry, + &runtime, + Some(callback), + ) + .await + .expect("query should retry and succeed"); + + assert_eq!( + tokio::time::Instant::now().duration_since(started_at), + std::time::Duration::from_secs(120) + ); + let retry_statuses = events + .lock() + .expect("lock events") + .iter() + .filter_map(|event| match event { + QueryEvent::ProviderRetryStatus(status) => Some(status.clone()), + QueryEvent::ContextCompactionStarted + | QueryEvent::ContextCompactionCompleted + | QueryEvent::ContextCompactionFailed { .. } + | QueryEvent::TextDelta(_) + | QueryEvent::ReasoningDelta(_) + | QueryEvent::ReasoningCompleted + | QueryEvent::UsageDelta { .. } + | QueryEvent::ToolUseStart { .. } + | QueryEvent::ToolExecutionStart { .. } + | QueryEvent::ToolProgress { .. } + | QueryEvent::ToolResult { .. } + | QueryEvent::TurnComplete { .. } + | QueryEvent::Usage { .. } => None, + }) + .collect::>(); + assert_eq!( + retry_statuses, + vec![ + ProviderRetryStatus { + provider: "rate-limited-stream-create-provider".to_string(), + model: model.clone(), + attempt: 1, + backoff_ms: 60_000, + phase: QueryProviderRetryPhase::Scheduled, + message: "Retrying provider request in 60.0s".to_string(), + }, + ProviderRetryStatus { + provider: "rate-limited-stream-create-provider".to_string(), + model: model.clone(), + attempt: 1, + backoff_ms: 0, + phase: QueryProviderRetryPhase::Resumed, + message: "Retrying provider request now".to_string(), + }, + ProviderRetryStatus { + provider: "rate-limited-stream-create-provider".to_string(), + model: model.clone(), + attempt: 2, + backoff_ms: 60_000, + phase: QueryProviderRetryPhase::Scheduled, + message: "Retrying provider request in 60.0s".to_string(), + }, + ProviderRetryStatus { + provider: "rate-limited-stream-create-provider".to_string(), + model, + attempt: 2, + backoff_ms: 0, + phase: QueryProviderRetryPhase::Resumed, + message: "Retrying provider request now".to_string(), + }, + ] + ); + assert_eq!(provider.attempts.load(Ordering::SeqCst), 3); + } + #[tokio::test(start_paused = true)] async fn query_cancels_stream_creation_retry_backoff() { let provider = Arc::new(TransientStreamCreateProvider { diff --git a/crates/core/tests/context_limit_compaction.rs b/crates/core/tests/context_limit_compaction.rs new file mode 100644 index 00000000..4be485aa --- /dev/null +++ b/crates/core/tests/context_limit_compaction.rs @@ -0,0 +1,152 @@ +use std::pin::Pin; +use std::sync::Arc; +use std::sync::Mutex; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; + +use anyhow::Result; +use async_trait::async_trait; +use devo_core::EventCallback; +use devo_core::Message; +use devo_core::Model; +use devo_core::ModelRequest; +use devo_core::ModelResponse; +use devo_core::QueryEvent; +use devo_core::ResponseContent; +use devo_core::SessionConfig; +use devo_core::SessionState; +use devo_core::StopReason; +use devo_core::StreamEvent; +use devo_core::TurnConfig; +use devo_core::Usage; +use devo_core::query; +use devo_core::tools::ToolRegistry; +use devo_core::tools::ToolRuntime; +use devo_provider::ModelProviderSDK; +use devo_provider::error::ProviderError; +use futures::Stream; +use pretty_assertions::assert_eq; + +const CONTEXT_LIMIT_MESSAGE: &str = "This model's maximum context length is 1048565 tokens. However, you request 1058357 tokens (674357 in the messages, 384000 in the completion). Please reduce the length of the messages or completion."; + +struct ContextLimitThenSuccessProvider { + stream_attempts: AtomicUsize, + compaction_calls: AtomicUsize, +} + +#[async_trait] +impl ModelProviderSDK for ContextLimitThenSuccessProvider { + async fn completion(&self, _request: ModelRequest) -> Result { + self.compaction_calls.fetch_add(1, Ordering::SeqCst); + Ok(ModelResponse { + id: "compaction-response".to_string(), + content: vec![ResponseContent::Text("Earlier work summary".to_string())], + stop_reason: Some(StopReason::EndTurn), + usage: Usage::default(), + metadata: Default::default(), + }) + } + + async fn completion_stream( + &self, + _request: ModelRequest, + ) -> Result> + Send>>> { + if self.stream_attempts.fetch_add(1, Ordering::SeqCst) == 0 { + return Err(anyhow::Error::new(ProviderError::ContextLimitError { + message: CONTEXT_LIMIT_MESSAGE.to_string(), + current_tokens: None, + limit: None, + })); + } + + Ok(Box::pin(futures::stream::iter([Ok( + StreamEvent::MessageDone { + response: ModelResponse { + id: "final-response".to_string(), + content: vec![ResponseContent::Text("done".to_string())], + stop_reason: Some(StopReason::EndTurn), + usage: Usage::default(), + metadata: Default::default(), + }, + }, + )]))) + } + + fn name(&self) -> &str { + "context-limit-then-success" + } +} + +#[derive(Debug, PartialEq, Eq)] +enum CompactionEvent { + Started, + Completed, +} + +#[tokio::test] +async fn context_limit_error_compacts_and_retries_query() { + let provider = Arc::new(ContextLimitThenSuccessProvider { + stream_attempts: AtomicUsize::new(0), + compaction_calls: AtomicUsize::new(0), + }); + let provider_sdk: Arc = provider.clone(); + let registry = Arc::new(ToolRegistry::new()); + let runtime = ToolRuntime::new_without_permissions(Arc::clone(®istry)); + let tempdir = tempfile::tempdir().expect("tempdir"); + let mut session = SessionState::new(SessionConfig::default(), tempdir.path().to_path_buf()); + session.push_message(Message::user("earlier user request")); + session.push_message(Message::assistant_text("earlier assistant response")); + session.push_message(Message::user("latest user request")); + let events = Arc::new(Mutex::new(Vec::new())); + let captured_events = Arc::clone(&events); + let callback: EventCallback = Arc::new(move |event| { + let captured_events = Arc::clone(&captured_events); + Box::pin(async move { + captured_events.lock().expect("lock events").push(event); + }) + }); + + query( + &mut session, + &TurnConfig::new(Model::default(), None), + provider_sdk, + registry, + &runtime, + Some(callback), + ) + .await + .expect("query should compact and retry"); + + let compaction_events = events + .lock() + .expect("lock events") + .iter() + .filter_map(|event| match event { + QueryEvent::ContextCompactionStarted => Some(CompactionEvent::Started), + QueryEvent::ContextCompactionCompleted => Some(CompactionEvent::Completed), + QueryEvent::ContextCompactionFailed { .. } + | QueryEvent::ProviderRetryStatus(_) + | QueryEvent::TextDelta(_) + | QueryEvent::ReasoningDelta(_) + | QueryEvent::ReasoningCompleted + | QueryEvent::UsageDelta { .. } + | QueryEvent::ToolUseStart { .. } + | QueryEvent::ToolExecutionStart { .. } + | QueryEvent::ToolProgress { .. } + | QueryEvent::ToolResult { .. } + | QueryEvent::TurnComplete { .. } + | QueryEvent::Usage { .. } => None, + }) + .collect::>(); + + assert_eq!( + compaction_events, + vec![CompactionEvent::Started, CompactionEvent::Completed] + ); + assert_eq!(provider.stream_attempts.load(Ordering::SeqCst), 2); + assert_eq!(provider.compaction_calls.load(Ordering::SeqCst), 1); + assert_eq!( + session.prompt_source_messages().last(), + Some(&Message::assistant_text("done")) + ); +} diff --git a/crates/provider/src/error.rs b/crates/provider/src/error.rs index 49a89d9f..0c0282b5 100644 --- a/crates/provider/src/error.rs +++ b/crates/provider/src/error.rs @@ -151,6 +151,33 @@ impl ProviderError { } } +pub(crate) fn context_limit_error( + message: String, + error_kind: Option<&str>, + error_code: Option<&str>, +) -> Option { + let normalized_message = message.to_ascii_lowercase(); + let message_matches = normalized_message.contains("maximum context length") + || normalized_message.contains("context_length_exceeded") + || normalized_message.contains("context_too_long") + || (normalized_message.contains("context window") + && (normalized_message.contains("exceeded") + || normalized_message.contains("too long"))); + let metadata_matches = [error_kind, error_code] + .into_iter() + .flatten() + .map(str::to_ascii_lowercase) + .any(|value| { + value.contains("context_length_exceeded") || value.contains("context_too_long") + }); + + (message_matches || metadata_matches).then_some(ProviderError::ContextLimitError { + message, + current_tokens: None, + limit: None, + }) +} + // ── Tests ─────────────────────────────────────────────────────────── #[cfg(test)] diff --git a/crates/provider/src/http.rs b/crates/provider/src/http.rs index 254b4ac8..7e25daa4 100644 --- a/crates/provider/src/http.rs +++ b/crates/provider/src/http.rs @@ -13,6 +13,8 @@ use std::sync::Mutex; use std::sync::OnceLock; use tracing::warn; +use crate::error::context_limit_error; + #[derive(Clone, Copy)] enum HttpClientKind { Request, @@ -151,6 +153,24 @@ pub(crate) async fn invalid_status_error( response_body = %response_body, "provider request failed" ); + let response_value = serde_json::from_str::(&response_body).ok(); + let message = response_value + .as_ref() + .and_then(|value| value.pointer("/error/message")) + .and_then(Value::as_str) + .unwrap_or(&response_body) + .to_string(); + let error_kind = response_value + .as_ref() + .and_then(|value| value.pointer("/error/type")) + .and_then(Value::as_str); + let error_code = response_value + .as_ref() + .and_then(|value| value.pointer("/error/code")) + .and_then(Value::as_str); + if let Some(error) = context_limit_error(message, error_kind, error_code) { + return anyhow::Error::new(error); + } anyhow::anyhow!( "{provider} {operation} error for model {model}: Invalid status code: {status}; response body: {response_body}" ) diff --git a/crates/provider/src/openai/error_payload.rs b/crates/provider/src/openai/error_payload.rs index b9e27eea..b9a72436 100644 --- a/crates/provider/src/openai/error_payload.rs +++ b/crates/provider/src/openai/error_payload.rs @@ -3,6 +3,7 @@ use serde::Deserialize; use serde_json::Value; use crate::error::ProviderError; +use crate::error::context_limit_error; #[derive(Debug, Deserialize)] struct OpenAIErrorEnvelope { @@ -31,6 +32,13 @@ pub(super) fn provider_error_from_payload( .unwrap_or_else(|| "OpenAI-compatible provider returned an error".to_string()); let status_code = payload.code.as_ref().and_then(status_code_from_value); let provider_name = Some("openai".to_string()); + if let Some(error) = context_limit_error( + message.clone(), + payload.kind.as_deref(), + payload.code.as_ref().and_then(Value::as_str), + ) { + return Some(error); + } Some(match status_code { Some(401 | 403) => ProviderError::AuthenticationError { @@ -175,4 +183,44 @@ mod tests { ] ); } + + #[test] + fn classifies_context_length_error_payload() { + let request = ModelRequest { + model_slug: ModelProfileKey::Generic, + model: "provider-model".to_string(), + system: None, + messages: Vec::new(), + max_tokens: 384_000, + tools: None, + hosted_tools: Vec::new(), + sampling: Default::default(), + request_thinking: None, + reasoning_effort: None, + extra_body: None, + }; + let message = "This model's maximum context length is 1048565 tokens. However, you request 1058357 tokens (674357 in the messages, 384000 in the completion). Please reduce the length of the messages or completion."; + + let error = provider_error_from_payload( + &json!({ + "error": { + "message": message, + "type": "invalid_request_error", + "code": "context_length_exceeded" + } + }), + &request, + ) + .expect("provider error"); + + assert_eq!( + serde_json::to_value(error).expect("serialize provider error"), + json!({ + "error_kind": "context_limit_error", + "message": message, + "current_tokens": null, + "limit": null + }) + ); + } } diff --git a/crates/provider/tests/openai_chat_completions_stream.rs b/crates/provider/tests/openai_chat_completions_stream.rs index eb74f5c9..fb9a7930 100644 --- a/crates/provider/tests/openai_chat_completions_stream.rs +++ b/crates/provider/tests/openai_chat_completions_stream.rs @@ -50,6 +50,44 @@ async fn chat_stream_reports_error_payload_returned_with_http_200() { assert!(stream.next().await.is_none()); } +#[tokio::test] +async fn chat_stream_classifies_http_400_context_length_error() { + let message = "This model's maximum context length is 1048565 tokens. However, you request 1058357 tokens (674357 in the messages, 384000 in the completion). Please reduce the length of the messages or completion."; + let body = json!({ + "error": { + "message": message, + "type": "invalid_request_error", + "code": "context_length_exceeded" + } + }) + .to_string(); + let base_url = spawn_http_server("400 Bad Request", "application/json", body).await; + let provider = OpenAIProvider::new(base_url); + + let mut stream = provider + .completion_stream(minimal_request()) + .await + .expect("openai chat stream"); + let error = stream + .next() + .await + .expect("provider error stream item") + .expect_err("HTTP 400 context limit must be an error"); + let provider_error = error + .downcast_ref::() + .expect("structured provider error"); + + assert_eq!( + serde_json::to_value(provider_error).expect("serialize provider error"), + json!({ + "error_kind": "context_limit_error", + "message": message, + "current_tokens": null, + "limit": null + }) + ); +} + #[tokio::test] async fn chat_stream_buffers_tool_arguments_until_identity_arrives() { let chunks = vec![ @@ -309,17 +347,22 @@ fn message_done_response(events: &[StreamEvent]) -> &devo_protocol::ModelRespons } async fn spawn_sse_server(chunks: Vec) -> String { + spawn_http_server("200 OK", "text/event-stream", chunks.concat()).await +} + +async fn spawn_http_server(status: &str, content_type: &str, body: String) -> String { let listener = TcpListener::bind("127.0.0.1:0") .await .expect("bind test server"); let addr = listener.local_addr().expect("local addr"); + let status = status.to_string(); + let content_type = content_type.to_string(); tokio::spawn(async move { let (mut socket, _) = listener.accept().await.expect("accept request"); read_http_request(&mut socket).await.expect("read request"); - let body = chunks.concat(); let response = format!( - "HTTP/1.1 200 OK\r\n\ - content-type: text/event-stream\r\n\ + "HTTP/1.1 {status}\r\n\ + content-type: {content_type}\r\n\ cache-control: no-cache\r\n\ content-length: {}\r\n\ connection: close\r\n\ diff --git a/crates/server/src/runtime/connection.rs b/crates/server/src/runtime/connection.rs index 7ef5dff2..acb523cb 100644 --- a/crates/server/src/runtime/connection.rs +++ b/crates/server/src/runtime/connection.rs @@ -24,6 +24,7 @@ use crate::acp_auth_required_response; use crate::acp_notification_from_server_event; use crate::devo_extension_inner_method; +use super::outbound::OutboundDeliveryPolicy; use super::outbound::OutboundFrame; use super::outbound::enqueue_outbound; use super::outbound::enqueue_outbound_notification; @@ -634,8 +635,13 @@ impl ServerRuntime { )) }; if let Some((outbound_tx, frame)) = notification { - let _ = enqueue_outbound_notification(&outbound_tx, frame, "connection_notifications") - .await; + let _ = enqueue_outbound_notification( + &outbound_tx, + frame, + OutboundDeliveryPolicy::BestEffort, + "connection_notifications", + ) + .await; } self.record_subagent_output_event(event).await; } @@ -653,6 +659,7 @@ impl ServerRuntime { event: ServerEvent, ) { debug_assert!(is_connection_local_notification(method)); + let delivery_policy = outbound_delivery_policy(&event); let notification = { let mut connections = self.connections.lock().await; let Some(connection) = connections.get_mut(&connection_id) else { @@ -670,8 +677,13 @@ impl ServerRuntime { )) }; if let Some((outbound_tx, frame)) = notification { - let _ = enqueue_outbound_notification(&outbound_tx, frame, "connection_notifications") - .await; + let _ = enqueue_outbound_notification( + &outbound_tx, + frame, + delivery_policy, + "connection_notifications", + ) + .await; } } @@ -687,6 +699,7 @@ impl ServerRuntime { return; } let session_id = event.session_id(); + let delivery_policy = outbound_delivery_policy(&event); let child_parent_by_session = self.child_parent_by_session().await; let notification = { let mut connections = self.connections.lock().await; @@ -705,8 +718,13 @@ impl ServerRuntime { )) }; if let Some((outbound_tx, frame)) = notification { - let _ = enqueue_outbound_notification(&outbound_tx, frame, "connection_notifications") - .await; + let _ = enqueue_outbound_notification( + &outbound_tx, + frame, + delivery_policy, + "connection_notifications", + ) + .await; } } @@ -721,6 +739,7 @@ impl ServerRuntime { // (visible in Burp) keeps flowing into a full event channel. let method = event.method_name(); let session_id = event.session_id(); + let delivery_policy = outbound_delivery_policy(&event); let child_parent_by_session = self.child_parent_by_session().await; let active_turn_connections = self.active_turns.connection_map().await; let notifications = { @@ -750,8 +769,13 @@ impl ServerRuntime { .collect::>() }; for (outbound_tx, frame) in notifications { - let _ = enqueue_outbound_notification(&outbound_tx, frame, "connection_notifications") - .await; + let _ = enqueue_outbound_notification( + &outbound_tx, + frame, + delivery_policy, + "connection_notifications", + ) + .await; } self.record_subagent_output_event(&event).await; } @@ -1060,6 +1084,47 @@ async fn remove_pending_client_request( } } +fn outbound_delivery_policy(event: &ServerEvent) -> OutboundDeliveryPolicy { + match event { + ServerEvent::ItemDelta { .. } + | ServerEvent::TurnUsageUpdated(_) + | ServerEvent::WorkspaceChangesUpdated(_) + | ServerEvent::ReferenceSearchUpdated(_) + | ServerEvent::CommandExecOutputDelta(_) => OutboundDeliveryPolicy::BestEffort, + ServerEvent::SessionStarted(_) + | ServerEvent::SessionTitleUpdated(_) + | ServerEvent::SessionCompactionStarted(_) + | ServerEvent::SessionCompactionCompleted(_) + | ServerEvent::SessionCompactionFailed(_) + | ServerEvent::SessionStatusChanged(_) + | ServerEvent::SessionArchived(_) + | ServerEvent::SessionUnarchived(_) + | ServerEvent::SessionClosed(_) + | ServerEvent::SessionDeleted(_) + | ServerEvent::TurnStarted(_) + | ServerEvent::TurnCompleted(_) + | ServerEvent::TurnInterrupted(_) + | ServerEvent::TurnFailed(_) + | ServerEvent::TurnPlanUpdated(_) + | ServerEvent::TurnDiffUpdated(_) + | ServerEvent::TurnProviderRetryStatus(_) + | ServerEvent::ToolCallStatusUpdated(_) + | ServerEvent::RequestUserInput(_) + | ServerEvent::InputQueueUpdated(_) + | ServerEvent::SteerAccepted(_) + | ServerEvent::MessageEditRecorded(_) + | ServerEvent::TurnSuperseded(_) + | ServerEvent::WorkspaceRestoreStarted(_) + | ServerEvent::WorkspaceRestoreCompleted(_) + | ServerEvent::ItemStarted(_) + | ServerEvent::ItemCompleted(_) + | ServerEvent::ServerRequestResolved(_) + | ServerEvent::ReferenceSearchCompleted(_) + | ServerEvent::ReferenceSearchFailed(_) + | ServerEvent::CommandExecExited(_) => OutboundDeliveryPolicy::Reliable, + } +} + fn session_activity_event_id(event: &ServerEvent) -> Option { match event { ServerEvent::ToolCallStatusUpdated(payload) => Some(payload.session_id), @@ -1312,6 +1377,46 @@ mod tests { ); } + #[test] + fn item_lifecycle_is_reliable_while_item_deltas_are_best_effort() { + let session_id = SessionId::new(); + let turn_id = TurnId::new(); + let item_id = ItemId::new(); + let context = EventContext { + session_id, + turn_id: Some(turn_id), + item_id: Some(item_id), + seq: 0, + }; + let events = [ + ServerEvent::ItemCompleted(ItemEventPayload { + context: context.clone(), + item: ItemEnvelope { + item_id, + item_kind: ItemKind::ContextCompaction, + payload: serde_json::json!({ "title": "Context compacted" }), + }, + }), + ServerEvent::ItemDelta { + delta_kind: ItemDeltaKind::AgentMessageDelta, + payload: ItemDeltaPayload { + context, + delta: "token".to_string(), + stream_index: None, + channel: None, + }, + }, + ]; + + assert_eq!( + events.map(|event| outbound_delivery_policy(&event)), + [ + OutboundDeliveryPolicy::Reliable, + OutboundDeliveryPolicy::BestEffort, + ] + ); + } + #[test] fn subscription_filter_can_match_direct_child_agents() { let parent = SessionId::new(); diff --git a/crates/server/src/runtime/outbound.rs b/crates/server/src/runtime/outbound.rs index c22bc14f..fad4ad7d 100644 --- a/crates/server/src/runtime/outbound.rs +++ b/crates/server/src/runtime/outbound.rs @@ -8,12 +8,19 @@ use tokio::sync::oneshot; use crate::NotificationEnvelope; pub(crate) const OUTBOUND_CHANNEL_CAPACITY: usize = 4096; +pub(crate) const OUTBOUND_RELIABLE_RESERVED_CAPACITY: usize = 64; pub(crate) const OUTBOUND_BACKPRESSURE_LOG_THRESHOLD: Duration = Duration::from_millis(50); /// Max time streaming notifications wait for outbound capacity before being /// dropped. Event streams must not park forever on a slow client: parent+child /// turns share one connection and can fill the queue quickly. pub(crate) const OUTBOUND_NOTIFICATION_MAX_WAIT: Duration = Duration::from_millis(200); +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum OutboundDeliveryPolicy { + Reliable, + BestEffort, +} + pub(crate) enum OutboundPayload { Notification { connection_id: u64, @@ -225,19 +232,49 @@ pub(crate) async fn enqueue_outbound( true } -/// Enqueue a streaming notification without risking an indefinite stall. +/// Enqueue a notification according to its delivery policy. /// -/// Unlike [`enqueue_outbound`], this gives up after -/// [`OUTBOUND_NOTIFICATION_MAX_WAIT`] and drops the frame. Turn event streams -/// (including child agents) call into `broadcast_event` while the session actor -/// is awaiting that stream; blocking here recreates the mailbox-style hang on -/// the outbound path when the client drains stdout slowly. +/// Best-effort notifications are accepted only when doing so preserves +/// [`OUTBOUND_RELIABLE_RESERVED_CAPACITY`] slots for lifecycle/control traffic. +/// Reliable notifications can use that reserved capacity and wait up to +/// [`OUTBOUND_NOTIFICATION_MAX_WAIT`] if reliable traffic fills the queue. This +/// keeps high-volume deltas from starving terminal events without allowing a +/// stalled client to park a turn event stream indefinitely. pub(crate) async fn enqueue_outbound_notification( tx: &mpsc::Sender, frame: OutboundFrame, + policy: OutboundDeliveryPolicy, queue: &'static str, ) -> bool { let connection_id = frame.connection_id(); + if policy == OutboundDeliveryPolicy::BestEffort { + let reserved_capacity = + OUTBOUND_RELIABLE_RESERVED_CAPACITY.min(tx.max_capacity().saturating_sub(1)); + let required_capacity = reserved_capacity + 1; + return match tx.try_reserve_many(required_capacity) { + Ok(mut permits) => { + permits + .next() + .expect("reserving positive capacity returns a permit") + .send(frame); + true + } + Err(tokio::sync::mpsc::error::TrySendError::Closed(())) => { + tracing::debug!(connection_id, queue, "outbound queue receiver dropped"); + false + } + Err(tokio::sync::mpsc::error::TrySendError::Full(())) => { + tracing::debug!( + connection_id, + queue, + reserved_capacity, + "shedding best-effort outbound notification under backpressure" + ); + false + } + }; + } + match tx.try_reserve() { Ok(permit) => { permit.send(frame); @@ -392,4 +429,81 @@ mod tests { let frame = OutboundFrame::json_rpc_response(1, response.clone()); assert_eq!(outbound_frame_to_value(&frame), response); } + + #[tokio::test] + async fn reliable_notification_uses_capacity_reserved_from_best_effort() { + let capacity = OUTBOUND_RELIABLE_RESERVED_CAPACITY + 1; + let (tx, mut rx) = mpsc::channel(capacity); + let best_effort = serde_json::json!({ + "context": { "item_id": "delta-item" }, + "delta": "token", + }); + assert!( + enqueue_outbound_notification( + &tx, + OutboundFrame::notification( + 1, + "item/agentMessage/delta".to_string(), + 1, + best_effort.clone(), + ), + OutboundDeliveryPolicy::BestEffort, + "test_notifications", + ) + .await + ); + assert!( + !enqueue_outbound_notification( + &tx, + OutboundFrame::notification( + 1, + "item/agentMessage/delta".to_string(), + 2, + best_effort, + ), + OutboundDeliveryPolicy::BestEffort, + "test_notifications", + ) + .await + ); + + let completed = serde_json::json!({ + "context": { "item_id": "compaction-item" }, + "item": { + "item_id": "compaction-item", + "item_kind": "context_compaction", + "payload": { "title": "Context compacted" }, + }, + }); + assert!( + enqueue_outbound_notification( + &tx, + OutboundFrame::notification(1, "item/completed".to_string(), 3, completed.clone(),), + OutboundDeliveryPolicy::Reliable, + "test_notifications", + ) + .await + ); + + let delivered = vec![ + outbound_frame_to_value(&rx.recv().await.expect("best-effort frame")), + outbound_frame_to_value(&rx.recv().await.expect("reliable frame")), + ]; + assert_eq!( + delivered, + vec![ + serde_json::json!({ + "method": "item/agentMessage/delta", + "params": { + "context": { "item_id": "delta-item" }, + "delta": "token", + }, + }), + serde_json::json!({ + "method": "item/completed", + "params": completed, + }), + ] + ); + } } diff --git a/crates/server/src/runtime/turn_exec/context_compaction.rs b/crates/server/src/runtime/turn_exec/context_compaction.rs new file mode 100644 index 00000000..748f8020 --- /dev/null +++ b/crates/server/src/runtime/turn_exec/context_compaction.rs @@ -0,0 +1,167 @@ +use std::sync::Arc; + +use devo_core::{ItemId, SessionId, TurnId}; + +use super::super::ServerRuntime; +use crate::{ + EventContext, ItemEnvelope, ItemEventPayload, ItemKind, ServerEvent, + SessionCompactionFailedPayload, +}; + +#[derive(Default)] +pub(super) struct ContextCompactionLifecycle { + item_id: Option, +} + +impl ContextCompactionLifecycle { + pub(super) async fn start( + &mut self, + runtime: &Arc, + session_id: SessionId, + turn_id: TurnId, + ) { + if self.item_id.is_some() { + self.fail( + runtime, + session_id, + turn_id, + "compaction restarted before the previous lifecycle completed".to_string(), + ) + .await; + } + let item_id = ItemId::new(); + self.item_id = Some(item_id); + runtime + .broadcast_event(started_event(session_id, turn_id, item_id)) + .await; + } + + pub(super) async fn complete( + &mut self, + runtime: &Arc, + session_id: SessionId, + turn_id: TurnId, + ) { + let Some(item_id) = self.item_id.take() else { + return; + }; + runtime + .broadcast_event(completed_event(session_id, turn_id, item_id)) + .await; + } + + pub(super) async fn fail( + &mut self, + runtime: &Arc, + session_id: SessionId, + turn_id: TurnId, + message: String, + ) { + if let Some(item_id) = self.item_id.take() { + for event in failed_events(session_id, turn_id, item_id, message) { + runtime.broadcast_event(event).await; + } + } else { + runtime + .broadcast_event(ServerEvent::SessionCompactionFailed( + SessionCompactionFailedPayload { + session_id, + message, + }, + )) + .await; + } + } + + pub(super) async fn close_if_open( + &mut self, + runtime: &Arc, + session_id: SessionId, + turn_id: TurnId, + ) { + if self.item_id.is_some() { + self.fail( + runtime, + session_id, + turn_id, + "compaction lifecycle ended before completion".to_string(), + ) + .await; + } + } +} + +pub(super) fn started_event( + session_id: SessionId, + turn_id: TurnId, + item_id: ItemId, +) -> ServerEvent { + item_event( + session_id, + turn_id, + item_id, + ServerEvent::ItemStarted, + serde_json::json!({ "title": "Compaction started" }), + ) +} + +pub(super) fn completed_event( + session_id: SessionId, + turn_id: TurnId, + item_id: ItemId, +) -> ServerEvent { + item_event( + session_id, + turn_id, + item_id, + ServerEvent::ItemCompleted, + serde_json::json!({ "title": "Context compacted" }), + ) +} + +pub(super) fn failed_events( + session_id: SessionId, + turn_id: TurnId, + item_id: ItemId, + message: String, +) -> [ServerEvent; 2] { + [ + item_event( + session_id, + turn_id, + item_id, + ServerEvent::ItemCompleted, + serde_json::json!({ + "title": "Compaction failed", + "status": "failed", + "message": message.clone(), + }), + ), + ServerEvent::SessionCompactionFailed(SessionCompactionFailedPayload { + session_id, + message, + }), + ] +} + +fn item_event( + session_id: SessionId, + turn_id: TurnId, + item_id: ItemId, + wrap: impl FnOnce(ItemEventPayload) -> ServerEvent, + payload: serde_json::Value, +) -> ServerEvent { + wrap(ItemEventPayload { + context: EventContext { + session_id, + turn_id: Some(turn_id), + item_id: Some(item_id), + seq: 0, + }, + item: ItemEnvelope { + item_id, + item_kind: ItemKind::ContextCompaction, + payload, + }, + }) +} diff --git a/crates/server/src/runtime/turn_exec/event_stream.rs b/crates/server/src/runtime/turn_exec/event_stream.rs index 63c6b223..b1670590 100644 --- a/crates/server/src/runtime/turn_exec/event_stream.rs +++ b/crates/server/src/runtime/turn_exec/event_stream.rs @@ -4,6 +4,7 @@ use devo_core::{ItemId, SessionId, TurnId}; use super::super::ServerRuntime; use super::super::proposed_plan::ProposedPlanParser; +use super::context_compaction::ContextCompactionLifecycle; use super::item_stream::{ ProposedPlanStreamItem, complete_assistant_item, complete_reasoning_item, handle_proposed_plan_segments, push_assistant_text_delta, @@ -14,8 +15,8 @@ use super::tool_display::{ use super::tool_results; use super::tool_results::complete_pending_tool_call; use super::trace::{ - query_event_trace_delta_len, query_event_trace_kind, query_event_trace_token_preview, - stream_trace_elapsed_ms, + QueryEventDeliveryPolicy, query_event_delivery_policy, query_event_trace_delta_len, + query_event_trace_kind, query_event_trace_token_preview, stream_trace_elapsed_ms, }; use super::types::{PendingToolCall, ToolDisplayKind, TurnEventStreamSummary}; use crate::runtime::session_actor::state::SessionStreamState; @@ -26,21 +27,16 @@ pub(crate) const QUERY_EVENT_CHANNEL_CAPACITY: usize = 8192; /// Enqueue a query event into the turn event stream. /// -/// Visible token events (`TextDelta` / `ReasoningDelta`), retry status, and `TurnComplete` -/// must not be dropped: when the channel is full we apply backpressure to the -/// provider reader with `send().await` so the TUI keeps receiving tokens. -/// Other events may be dropped under pressure so coordination cannot wedge +/// Token deltas and lifecycle/control events must not be dropped: when the +/// channel is full they apply backpressure to the producer. High-volume tool +/// progress and usage updates remain best effort so they cannot wedge the turn /// forever on a stalled consumer. pub(super) async fn enqueue_query_event( event_tx: &mpsc::Sender, event: devo_core::QueryEvent, ) { let kind = query_event_trace_kind(&event); - let must_deliver = matches!( - kind, - "text_delta" | "reasoning_delta" | "provider_retry_status" | "turn_complete" - ); - if must_deliver { + if query_event_delivery_policy(&event) == QueryEventDeliveryPolicy::MustDeliver { let _ = event_tx.send(event).await; return; } @@ -90,6 +86,7 @@ pub(crate) fn spawn_turn_event_stream( let mut turn_usage = None; let mut latest_query_usage = None; let mut stop_reason = None; + let mut context_compaction = ContextCompactionLifecycle::default(); while let Some(event) = event_rx.recv().await { log_dequeued_query_event(&event); match event { @@ -116,6 +113,21 @@ pub(crate) fn spawn_turn_event_stream( )) .await; } + devo_core::QueryEvent::ContextCompactionStarted => { + context_compaction + .start(&runtime, session_id, turn_for_events.turn_id) + .await; + } + devo_core::QueryEvent::ContextCompactionCompleted => { + context_compaction + .complete(&runtime, session_id, turn_for_events.turn_id) + .await; + } + devo_core::QueryEvent::ContextCompactionFailed { message } => { + context_compaction + .fail(&runtime, session_id, turn_for_events.turn_id, message) + .await; + } devo_core::QueryEvent::TextDelta(text) => { if let Some(parser) = proposed_plan_parser.as_mut() { let segments = parser.push_str(&text); @@ -310,6 +322,9 @@ pub(crate) fn spawn_turn_event_stream( } } } + context_compaction + .close_if_open(&runtime, session_id, turn_for_events.turn_id) + .await; finish_proposed_plan_stream( &runtime, &event_stream, diff --git a/crates/server/src/runtime/turn_exec/mod.rs b/crates/server/src/runtime/turn_exec/mod.rs index 9e57cdd1..141eff35 100644 --- a/crates/server/src/runtime/turn_exec/mod.rs +++ b/crates/server/src/runtime/turn_exec/mod.rs @@ -1,3 +1,4 @@ +mod context_compaction; mod event_stream; mod failure; mod finalize; diff --git a/crates/server/src/runtime/turn_exec/tests.rs b/crates/server/src/runtime/turn_exec/tests.rs index 712d8245..bb385744 100644 --- a/crates/server/src/runtime/turn_exec/tests.rs +++ b/crates/server/src/runtime/turn_exec/tests.rs @@ -1,12 +1,15 @@ -use super::tool_display::*; +use super::tool_display::*; use super::types::*; use crate::*; -use devo_core::ItemId; use devo_core::tools::tool_spec::ToolPreparationFeedback; +use devo_core::{ItemId, SessionId, TurnId}; use pretty_assertions::assert_eq; use std::collections::HashMap; +use super::context_compaction::{completed_event, failed_events, started_event}; use super::event_stream::enqueue_query_event; +use super::trace::QueryEventDeliveryPolicy; +use super::trace::query_event_delivery_policy; #[test] fn command_progress_uses_command_execution_item_id() { @@ -48,6 +51,83 @@ fn command_progress_uses_command_execution_item_id() { ); } +#[test] +fn context_compaction_events_share_stable_item_lifecycle() { + let session_id = SessionId::new(); + let turn_id = TurnId::new(); + let item_id = ItemId::new(); + + assert_eq!( + vec![ + started_event(session_id, turn_id, item_id), + completed_event(session_id, turn_id, item_id), + ], + vec![ + ServerEvent::ItemStarted(ItemEventPayload { + context: EventContext { + session_id, + turn_id: Some(turn_id), + item_id: Some(item_id), + seq: 0, + }, + item: ItemEnvelope { + item_id, + item_kind: ItemKind::ContextCompaction, + payload: serde_json::json!({ "title": "Compaction started" }), + }, + }), + ServerEvent::ItemCompleted(ItemEventPayload { + context: EventContext { + session_id, + turn_id: Some(turn_id), + item_id: Some(item_id), + seq: 0, + }, + item: ItemEnvelope { + item_id, + item_kind: ItemKind::ContextCompaction, + payload: serde_json::json!({ "title": "Context compacted" }), + }, + }), + ] + ); +} + +#[test] +fn context_compaction_failure_closes_item_and_reports_visible_error() { + let session_id = SessionId::new(); + let turn_id = TurnId::new(); + let item_id = ItemId::new(); + let message = "context limit".to_string(); + + assert_eq!( + failed_events(session_id, turn_id, item_id, message.clone()), + [ + ServerEvent::ItemCompleted(ItemEventPayload { + context: EventContext { + session_id, + turn_id: Some(turn_id), + item_id: Some(item_id), + seq: 0, + }, + item: ItemEnvelope { + item_id, + item_kind: ItemKind::ContextCompaction, + payload: serde_json::json!({ + "title": "Compaction failed", + "status": "failed", + "message": message, + }), + }, + }), + ServerEvent::SessionCompactionFailed(SessionCompactionFailedPayload { + session_id, + message, + }), + ] + ); +} + #[test] fn file_change_tool_detection_matches_apply_patch_and_write() { assert!(is_file_change_tool("apply_patch")); @@ -408,3 +488,90 @@ async fn provider_retry_status_waits_for_channel_capacity() { }; assert_eq!(received_status, retry_status); } + +#[test] +fn lifecycle_and_control_query_events_are_must_deliver() { + let events = [ + devo_core::QueryEvent::ContextCompactionStarted, + devo_core::QueryEvent::ContextCompactionCompleted, + devo_core::QueryEvent::ContextCompactionFailed { + message: "context limit".to_string(), + }, + devo_core::QueryEvent::ReasoningCompleted, + devo_core::QueryEvent::ToolUseStart { + id: "tool-1".to_string(), + name: "read".to_string(), + input: serde_json::json!({ "path": "README.md" }), + }, + devo_core::QueryEvent::ToolExecutionStart { + id: "tool-1".to_string(), + }, + devo_core::QueryEvent::ToolResult { + tool_use_id: "tool-1".to_string(), + tool_name: "read".to_string(), + input: serde_json::json!({ "path": "README.md" }), + content: devo_core::tools::ToolContent::Text("contents".to_string()), + display_content: None, + is_error: false, + summary: "read README.md".to_string(), + }, + devo_core::QueryEvent::ToolProgress { + tool_use_id: "tool-1".to_string(), + progress: devo_core::tools::ToolProgress::Terminal { + terminal_id: "terminal-1".to_string(), + }, + }, + devo_core::QueryEvent::TextDelta("text".to_string()), + devo_core::QueryEvent::ReasoningDelta("reasoning".to_string()), + devo_core::QueryEvent::TurnComplete { + stop_reason: devo_core::StopReason::EndTurn, + }, + ]; + + assert_eq!( + events + .iter() + .map(query_event_delivery_policy) + .collect::>(), + vec![QueryEventDeliveryPolicy::MustDeliver; events.len()] + ); +} + +#[test] +fn high_volume_query_events_are_best_effort() { + let events = [ + devo_core::QueryEvent::ToolProgress { + tool_use_id: "tool-1".to_string(), + progress: devo_core::tools::ToolProgress::OutputDelta { + delta: "output".to_string(), + }, + }, + devo_core::QueryEvent::ToolProgress { + tool_use_id: "tool-1".to_string(), + progress: devo_core::tools::ToolProgress::StatusUpdate { + message: "working".to_string(), + percent: Some(50), + }, + }, + devo_core::QueryEvent::ToolProgress { + tool_use_id: "tool-1".to_string(), + progress: devo_core::tools::ToolProgress::Completion { + summary: "done".to_string(), + }, + }, + devo_core::QueryEvent::UsageDelta { + usage: devo_protocol::Usage::default(), + }, + devo_core::QueryEvent::Usage { + usage: devo_protocol::Usage::default(), + }, + ]; + + assert_eq!( + events + .iter() + .map(query_event_delivery_policy) + .collect::>(), + vec![QueryEventDeliveryPolicy::BestEffort; events.len()] + ); +} diff --git a/crates/server/src/runtime/turn_exec/trace.rs b/crates/server/src/runtime/turn_exec/trace.rs index 26b1194c..1f843e25 100644 --- a/crates/server/src/runtime/turn_exec/trace.rs +++ b/crates/server/src/runtime/turn_exec/trace.rs @@ -3,6 +3,41 @@ use std::time::Instant; use devo_core::QueryEvent; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) enum QueryEventDeliveryPolicy { + MustDeliver, + BestEffort, +} + +pub(super) fn query_event_delivery_policy(event: &QueryEvent) -> QueryEventDeliveryPolicy { + match event { + QueryEvent::ToolProgress { + progress: + devo_core::tools::ToolProgress::OutputDelta { .. } + | devo_core::tools::ToolProgress::StatusUpdate { .. } + | devo_core::tools::ToolProgress::Completion { .. }, + .. + } + | QueryEvent::UsageDelta { .. } + | QueryEvent::Usage { .. } => QueryEventDeliveryPolicy::BestEffort, + QueryEvent::ProviderRetryStatus(_) + | QueryEvent::ContextCompactionStarted + | QueryEvent::ContextCompactionCompleted + | QueryEvent::ContextCompactionFailed { .. } + | QueryEvent::TextDelta(_) + | QueryEvent::ReasoningDelta(_) + | QueryEvent::ReasoningCompleted + | QueryEvent::ToolUseStart { .. } + | QueryEvent::ToolExecutionStart { .. } + | QueryEvent::ToolResult { .. } + | QueryEvent::ToolProgress { + progress: devo_core::tools::ToolProgress::Terminal { .. }, + .. + } + | QueryEvent::TurnComplete { .. } => QueryEventDeliveryPolicy::MustDeliver, + } +} + pub(super) fn stream_trace_elapsed_ms() -> u128 { static STREAM_TRACE_START: OnceLock = OnceLock::new(); STREAM_TRACE_START @@ -14,6 +49,9 @@ pub(super) fn stream_trace_elapsed_ms() -> u128 { pub(super) fn query_event_trace_kind(event: &QueryEvent) -> &'static str { match event { QueryEvent::ProviderRetryStatus(_) => "provider_retry_status", + QueryEvent::ContextCompactionStarted => "context_compaction_started", + QueryEvent::ContextCompactionCompleted => "context_compaction_completed", + QueryEvent::ContextCompactionFailed { .. } => "context_compaction_failed", QueryEvent::TextDelta(_) => "text_delta", QueryEvent::ReasoningDelta(_) => "reasoning_delta", QueryEvent::ReasoningCompleted => "reasoning_completed", @@ -42,6 +80,9 @@ pub(super) fn query_event_trace_delta_len(event: &QueryEvent) -> usize { .. } | QueryEvent::ProviderRetryStatus(_) + | QueryEvent::ContextCompactionStarted + | QueryEvent::ContextCompactionCompleted + | QueryEvent::ContextCompactionFailed { .. } | QueryEvent::ReasoningCompleted | QueryEvent::ToolUseStart { .. } | QueryEvent::ToolExecutionStart { .. } @@ -55,7 +96,19 @@ pub(super) fn query_event_trace_delta_len(event: &QueryEvent) -> usize { pub(super) fn query_event_trace_token_preview(event: &QueryEvent) -> Option { match event { QueryEvent::TextDelta(text) => assistant_token_log_preview(text), - _ => None, + QueryEvent::ProviderRetryStatus(_) + | QueryEvent::ContextCompactionStarted + | QueryEvent::ContextCompactionCompleted + | QueryEvent::ContextCompactionFailed { .. } + | QueryEvent::ReasoningDelta(_) + | QueryEvent::ReasoningCompleted + | QueryEvent::ToolUseStart { .. } + | QueryEvent::ToolExecutionStart { .. } + | QueryEvent::ToolProgress { .. } + | QueryEvent::ToolResult { .. } + | QueryEvent::TurnComplete { .. } + | QueryEvent::UsageDelta { .. } + | QueryEvent::Usage { .. } => None, } } diff --git a/crates/tui/src/chatwidget/text_stream.rs b/crates/tui/src/chatwidget/text_stream.rs index d03ac3a2..165133e1 100644 --- a/crates/tui/src/chatwidget/text_stream.rs +++ b/crates/tui/src/chatwidget/text_stream.rs @@ -95,6 +95,10 @@ impl ChatWidget { return; } + if kind == TextItemKind::Reasoning { + self.commit_completed_assistant_before_next_reasoning(); + } + let seq = self.reserve_seq(); let stream_controller = if kind == TextItemKind::Assistant { Some(StreamController::new(None, &self.session.cwd)) @@ -378,6 +382,25 @@ impl ChatWidget { } } + fn commit_completed_assistant_before_next_reasoning(&mut self) { + self.commit_completed_text_items(); + while let Some(assistant_index) = self.active_text_items.iter().position(|item| { + item.kind == TextItemKind::Assistant && item.status == DotStatus::Completed + }) { + let Some(reasoning_index) = + self.active_text_items[..assistant_index] + .iter() + .position(|item| { + item.kind == TextItemKind::Reasoning && item.status == DotStatus::Pending + }) + else { + break; + }; + self.commit_text_item_at(reasoning_index, DotStatus::Completed); + } + self.commit_completed_text_items(); + } + fn active_text_item_log_order(&self) -> Vec { self.active_text_items .iter() diff --git a/crates/tui/src/chatwidget/transcript_view.rs b/crates/tui/src/chatwidget/transcript_view.rs index bfd9348c..684e6f15 100644 --- a/crates/tui/src/chatwidget/transcript_view.rs +++ b/crates/tui/src/chatwidget/transcript_view.rs @@ -301,13 +301,17 @@ impl ChatWidget { tool_call.output.clone(), ) .display_lines(width), - _ => history_cell::AgentMessageCell::new_with_prefix( - tool_call.lines.clone(), - Self::pending_dot_prefix(), - " ", - false, - ) - .display_lines(width), + _ => { + let mut lines = vec![Self::running_tool_line(&tool_call.title)]; + lines.extend(tool_call.lines.clone()); + history_cell::AgentMessageCell::new_with_prefix( + lines, + Self::pending_dot_prefix(), + " ", + false, + ) + .display_lines(width) + } } } @@ -329,13 +333,17 @@ impl ChatWidget { tool_call.output.clone(), ) .transcript_lines(width), - _ => history_cell::AgentMessageCell::new_with_prefix( - tool_call.lines.clone(), - Self::pending_dot_prefix(), - " ", - false, - ) - .transcript_lines(width), + _ => { + let mut lines = vec![Self::running_tool_line(&tool_call.title)]; + lines.extend(tool_call.lines.clone()); + history_cell::AgentMessageCell::new_with_prefix( + lines, + Self::pending_dot_prefix(), + " ", + false, + ) + .transcript_lines(width) + } } } diff --git a/crates/tui/src/chatwidget/worker_events.rs b/crates/tui/src/chatwidget/worker_events.rs index 0ab3df9f..00decda6 100644 --- a/crates/tui/src/chatwidget/worker_events.rs +++ b/crates/tui/src/chatwidget/worker_events.rs @@ -30,7 +30,6 @@ use super::ChatWidget; use super::DotStatus; use super::PendingApprovalRequest; use super::SKILLS_TRANSCRIPT_TITLE; -use super::session_header::is_web_search_title; use super::text_stream::ActiveTextItemId; fn format_retry_status_message(attempt: usize, backoff_ms: u64) -> String { @@ -53,6 +52,24 @@ impl ChatWidget { self.current_turn_mode = InputMode::Shell; } + let already_rendered = self + .active_cell + .as_ref() + .and_then(|cell| cell.as_any().downcast_ref::()) + .is_some_and(|cell| cell.contains_call(&tool_use_id)) + || self.history.iter().any(|cell| { + cell.as_any() + .downcast_ref::() + .is_some_and(|cell| cell.contains_call(&tool_use_id)) + }); + if already_rendered { + return; + } + + self.active_tool_calls.remove(&tool_use_id); + self.pending_tool_calls + .retain(|pending| pending.tool_use_id != tool_use_id); + if let Some(cell) = self .active_cell .as_mut() @@ -328,6 +345,9 @@ impl ChatWidget { start_time: None, }; if preparing { + self.active_tool_calls.remove(&tool_use_id); + self.pending_tool_calls + .retain(|pending| pending.tool_use_id != tool_use_id); self.pending_tool_calls.push(ActiveToolCall { lines: Vec::new(), start_time: Some(Instant::now()), @@ -343,24 +363,6 @@ impl ChatWidget { .retain(|pc| self.active_tool_calls.contains_key(&pc.tool_use_id)); self.active_tool_calls .insert(tool_use_id.clone(), tool_call); - let pending_title = - if title.starts_with("Running ") || is_web_search_title(&title) { - title - } else { - format!("Running {title}") - }; - let seq = self.reserve_seq(); - self.pending_tool_calls.push(ActiveToolCall { - tool_use_id, - seq, - tool_name: None, - input: None, - title: pending_title, - lines: Vec::new(), - output: String::new(), - exec_like: false, - start_time: Some(Instant::now()), - }); } self.active_cell_revision = self.active_cell_revision.wrapping_add(1); self.frame_requester.schedule_frame(); @@ -502,13 +504,8 @@ impl ChatWidget { is_error, truncated, } => { - if let Some(pos) = self - .pending_tool_calls - .iter() - .position(|tc| tc.tool_use_id == tool_use_id) - { - self.pending_tool_calls.remove(pos); - } + self.pending_tool_calls + .retain(|pending| pending.tool_use_id != tool_use_id); let dot_status = if is_error { DotStatus::Failed } else { @@ -631,13 +628,8 @@ impl ChatWidget { truncated, } => { // Remove from pending viewport entries — it will be committed to history below. - if let Some(pos) = self - .pending_tool_calls - .iter() - .position(|tc| tc.tool_use_id == tool_use_id) - { - self.pending_tool_calls.remove(pos); - } + self.pending_tool_calls + .retain(|pending| pending.tool_use_id != tool_use_id); let dot_status = if is_error { DotStatus::Failed } else { @@ -1321,6 +1313,12 @@ impl ChatWidget { self.set_status_message("Session renamed"); } WorkerEvent::SessionCompactionStarted => { + if self.status_message != "Session compaction in progress" { + self.add_to_history(history_cell::new_live_aligned_info_event( + "Compaction started".to_string(), + None, + )); + } self.busy = true; self.bottom_pane.set_task_running(true); if let Some(status) = self.bottom_pane.status_widget_mut() { @@ -1343,23 +1341,34 @@ impl ChatWidget { self.last_query_total_tokens = last_query_total_tokens; self.last_query_input_tokens = last_query_input_tokens; self.prompt_token_estimate = prompt_token_estimate; - self.add_to_history(history_cell::new_live_aligned_info_event( - "Session compaction done".to_string(), - None, - )); + if self.status_message != "Context compacted" { + self.add_to_history(history_cell::new_live_aligned_info_event( + "Context compacted".to_string(), + None, + )); + } self.set_status_message("Session compacted"); } - WorkerEvent::ContextCompactionCompleted { title } => { - self.add_to_history(history_cell::new_live_aligned_info_event(title, None)); + WorkerEvent::ContextCompactionCompleted { title: _ } => { + if self.status_message != "Session compacted" + && self.status_message != "Context compacted" + { + self.add_to_history(history_cell::new_live_aligned_info_event( + "Context compacted".to_string(), + None, + )); + } self.set_status_message("Context compacted"); } WorkerEvent::SessionCompactionFailed { message } => { self.busy = false; self.bottom_pane.set_task_running(false); - self.add_to_history(history_cell::new_live_aligned_error_event_with_hint( - message, - Some("session compaction failed".to_string()), - )); + if self.status_message != "Session compaction failed" { + self.add_to_history(history_cell::new_live_aligned_error_event_with_hint( + message, + Some("session compaction failed".to_string()), + )); + } self.set_status_message("Session compaction failed"); } WorkerEvent::SessionTitleUpdated { diff --git a/crates/tui/src/chatwidget_tests.rs b/crates/tui/src/chatwidget_tests.rs index a80b05c1..c7e854ec 100644 --- a/crates/tui/src/chatwidget_tests.rs +++ b/crates/tui/src/chatwidget_tests.rs @@ -5493,6 +5493,72 @@ fn lifecycle_text_items_keep_reasoning_before_assistant_when_events_arrive_out_o ); } +#[test] +fn completed_assistant_flushes_before_next_reasoning_starts() { + let cwd = std::env::current_dir().expect("current directory is available"); + let model = Model { + slug: "test-model".to_string(), + display_name: "Test Model".to_string(), + ..Model::default() + }; + let (mut widget, _app_event_rx) = widget_with_model(model, cwd); + let stale_reasoning_id = ItemId::new(); + let assistant_id = ItemId::new(); + let next_reasoning_id = ItemId::new(); + + widget.handle_worker_event(crate::events::WorkerEvent::TextItemStarted { + item_id: stale_reasoning_id, + kind: crate::events::TextItemKind::Reasoning, + }); + widget.handle_worker_event(crate::events::WorkerEvent::TextItemDelta { + item_id: stale_reasoning_id, + kind: crate::events::TextItemKind::Reasoning, + delta: "first thought".to_string(), + }); + widget.handle_worker_event(crate::events::WorkerEvent::TextItemStarted { + item_id: assistant_id, + kind: crate::events::TextItemKind::Assistant, + }); + widget.handle_worker_event(crate::events::WorkerEvent::TextItemDelta { + item_id: assistant_id, + kind: crate::events::TextItemKind::Assistant, + delta: "first answer".to_string(), + }); + widget.handle_worker_event(crate::events::WorkerEvent::TextItemCompleted { + item_id: assistant_id, + kind: crate::events::TextItemKind::Assistant, + final_text: "first answer".to_string(), + }); + widget.handle_worker_event(crate::events::WorkerEvent::TextItemStarted { + item_id: next_reasoning_id, + kind: crate::events::TextItemKind::Reasoning, + }); + widget.handle_worker_event(crate::events::WorkerEvent::TextItemDelta { + item_id: next_reasoning_id, + kind: crate::events::TextItemKind::Reasoning, + delta: "second thought".to_string(), + }); + + let committed = scrollback_plain_lines(&widget.drain_scrollback_lines(100)).join("\n"); + let first_thought_index = committed + .find("first thought") + .expect("stale reasoning should be reconciled"); + let first_answer_index = committed + .find("first answer") + .expect("completed assistant should flush"); + assert!( + first_thought_index < first_answer_index, + "reconciled reasoning should remain before its assistant:\n{committed}" + ); + assert_eq!(committed.matches("first thought").count(), 1, "{committed}"); + assert_eq!(committed.matches("first answer").count(), 1, "{committed}"); + + let active = line_texts(widget.active_viewport_lines_for_test(100)).join("\n"); + assert!(active.contains("Thinking: second thought"), "{active}"); + assert!(!active.contains("first thought"), "{active}"); + assert!(!active.contains("first answer"), "{active}"); +} + #[test] fn assistant_stream_commit_tick_runs_while_reasoning_is_pending() { let cwd = std::env::current_dir().expect("current directory is available"); @@ -6171,6 +6237,15 @@ fn session_compaction_live_rows_use_live_prefix_cols() { widget.handle_worker_event(crate::events::WorkerEvent::SessionCompactionStarted); + let started_history = scrollback_plain_lines(&widget.drain_scrollback_lines(80)); + assert!( + started_history + .iter() + .any(|line| line.starts_with(&format!("{live_prefix}Compaction started"))), + "context compaction start should be visible in history:\n{}", + started_history.join("\n") + ); + let rows = rendered_rows(&widget, 120, 24); assert!( rows.iter() @@ -6192,7 +6267,7 @@ fn session_compaction_live_rows_use_live_prefix_cols() { assert!( history .iter() - .any(|line| { line.starts_with(&format!("{live_prefix}Session compaction done")) }), + .any(|line| { line.starts_with(&format!("{live_prefix}Context compacted")) }), "compaction completion history should align with live prefix:\n{}", history.join("\n") ); @@ -6204,8 +6279,8 @@ fn session_compaction_live_rows_use_live_prefix_cols() { assert!( history .iter() - .any(|line| { line.starts_with(&format!("{live_prefix}Context compacted for turn")) }), - "context compaction history should align with live prefix:\n{}", + .all(|line| !line.contains("Context compacted")), + "paired session/item completion should not duplicate history:\n{}", history.join("\n") ); @@ -6222,6 +6297,80 @@ fn session_compaction_live_rows_use_live_prefix_cols() { ); } +#[test] +fn context_compaction_item_lifecycle_emits_worker_events() { + let session_id = SessionId::new(); + let turn_id = TurnId::new(); + let item_id = ItemId::new(); + let context = devo_server::EventContext { + session_id, + turn_id: Some(turn_id), + item_id: Some(item_id), + seq: 1, + }; + let item = devo_server::ItemEnvelope { + item_id, + item_kind: devo_server::ItemKind::ContextCompaction, + payload: serde_json::json!({"title": "Context Compaction"}), + }; + let (event_tx, mut event_rx) = mpsc::unbounded_channel(); + + crate::worker::handle_started_item( + devo_server::ItemEventPayload { + context: context.clone(), + item: item.clone(), + }, + &event_tx, + ); + crate::worker::handle_completed_item( + devo_server::ItemEventPayload { context, item }, + &event_tx, + ); + + assert_eq!( + event_rx.try_recv().expect("compaction start event"), + crate::events::WorkerEvent::SessionCompactionStarted + ); + assert_eq!( + event_rx.try_recv().expect("compaction completion event"), + crate::events::WorkerEvent::ContextCompactionCompleted { + title: "Context Compaction".to_string() + } + ); +} + +#[test] +fn failed_context_compaction_item_emits_failure_event() { + let (event_tx, mut event_rx) = mpsc::unbounded_channel(); + + crate::worker::handle_completed_item( + devo_server::ItemEventPayload { + context: devo_server::EventContext { + session_id: SessionId::new(), + turn_id: Some(TurnId::new()), + item_id: None, + seq: 1, + }, + item: devo_server::ItemEnvelope { + item_id: ItemId::new(), + item_kind: devo_server::ItemKind::ContextCompaction, + payload: serde_json::json!({ + "title": "Compaction failed", + "message": "summary generation failed" + }), + }, + }, + &event_tx, + ); + + assert_eq!( + event_rx.try_recv().expect("compaction failure event"), + crate::events::WorkerEvent::SessionCompactionFailed { + message: "summary generation failed".to_string() + } + ); +} + #[test] fn request_user_input_and_subagent_live_list_are_visible_together() { let model = Model { @@ -7495,6 +7644,72 @@ fn transcript_overlay_lines_include_running_tool_input_and_output_delta() { ); } +#[test] +fn generic_tool_call_has_one_running_render_owner() { + let model = Model { + slug: "test-model".to_string(), + display_name: "Test Model".to_string(), + ..Model::default() + }; + let (mut widget, _app_event_rx) = widget_with_model(model, PathBuf::from(".")); + + widget.handle_worker_event(crate::events::WorkerEvent::ToolCall { + tool_use_id: "tool-1".to_string(), + summary: "custom job".to_string(), + preparing: false, + parsed_commands: None, + }); + widget.handle_worker_event(crate::events::WorkerEvent::ToolCallDetails { + tool_use_id: "tool-1".to_string(), + tool_name: "custom_tool".to_string(), + input: serde_json::json!({"target": "crate"}), + }); + + let active = line_texts(widget.active_viewport_lines_for_test(100)).join("\n"); + assert_eq!( + active.matches("Running custom job").count(), + 1, + "one tool_use_id should have one live render owner:\n{active}" + ); + + widget.handle_worker_event(crate::events::WorkerEvent::ToolResult { + tool_use_id: "tool-1".to_string(), + title: "custom job".to_string(), + preview: "done".to_string(), + is_error: false, + truncated: false, + }); + let active = line_texts(widget.active_viewport_lines_for_test(100)).join("\n"); + assert!(!active.contains("Running custom job"), "{active}"); +} + +#[test] +fn duplicate_command_execution_start_is_idempotent() { + let model = Model { + slug: "test-model".to_string(), + display_name: "Test Model".to_string(), + ..Model::default() + }; + let (mut widget, _app_event_rx) = widget_with_model(model, PathBuf::from(".")); + let started = crate::events::WorkerEvent::CommandExecutionStarted { + tool_use_id: "command-1".to_string(), + command: "pwd".to_string(), + input: None, + source: devo_protocol::protocol::ExecCommandSource::Agent, + command_actions: Vec::new(), + }; + + widget.handle_worker_event(started.clone()); + widget.handle_worker_event(started); + + let transcript = line_texts(widget.transcript_overlay_lines(100)).join("\n"); + assert_eq!( + transcript.matches("pwd").count(), + 1, + "duplicate starts should retain one command cell:\n{transcript}" + ); +} + #[test] fn transcript_overlay_lines_include_completed_tool_input_and_full_output() { let model = Model { diff --git a/crates/tui/src/events.rs b/crates/tui/src/events.rs index 3eb4564c..7fe955d7 100644 --- a/crates/tui/src/events.rs +++ b/crates/tui/src/events.rs @@ -587,7 +587,7 @@ pub(crate) enum WorkerEvent { /// The new session title. title: String, }, - /// The active session started a proactive compaction request. + /// The active session or its context-compaction transcript item started compaction. SessionCompactionStarted, /// The active session completed a proactive compaction request. SessionCompacted { @@ -606,7 +606,7 @@ pub(crate) enum WorkerEvent { }, /// A context-compaction transcript item was completed. ContextCompactionCompleted { - /// Display title for the compaction item. + /// Server-provided title retained for event compatibility. title: String, }, /// The active session compaction request failed. diff --git a/crates/tui/src/exec_cell/model.rs b/crates/tui/src/exec_cell/model.rs index 9812b4af..b2d027b3 100644 --- a/crates/tui/src/exec_cell/model.rs +++ b/crates/tui/src/exec_cell/model.rs @@ -174,6 +174,10 @@ impl ExecCell { self.calls.iter() } + pub(crate) fn contains_call(&self, call_id: &str) -> bool { + self.calls.iter().any(|call| call.call_id == call_id) + } + pub(crate) fn append_output(&mut self, call_id: &str, chunk: &str) -> bool { if chunk.is_empty() { return false; diff --git a/crates/tui/src/worker.rs b/crates/tui/src/worker.rs index df95bd7c..79f42b4e 100644 --- a/crates/tui/src/worker.rs +++ b/crates/tui/src/worker.rs @@ -2366,68 +2366,7 @@ async fn run_worker_inner( } "item/started" => { if let ServerEvent::ItemStarted(payload) = event { - tracing::debug!( - item_id = %payload.item.item_id, - item_kind = ?payload.item.item_kind, - "server item started" - ); - match payload.item.item_kind { - ItemKind::AgentMessage => { - let _ = event_tx.send(WorkerEvent::TextItemStarted { - item_id: payload.item.item_id, - kind: TextItemKind::Assistant, - }); - } - ItemKind::Reasoning => { - let _ = event_tx.send(WorkerEvent::TextItemStarted { - item_id: payload.item.item_id, - kind: TextItemKind::Reasoning, - }); - } - ItemKind::Plan => { - if is_proposed_plan_payload(&payload.item.payload) { - let _ = event_tx.send( - WorkerEvent::ProposedPlanStarted { - item_id: payload.item.item_id, - }, - ); - } - } - ItemKind::CommandExecution => { - if let Ok(payload) = - serde_json::from_value::( - payload.item.payload, - ) - { - let _ = event_tx.send( - WorkerEvent::CommandExecutionStarted { - tool_use_id: payload.tool_call_id, - command: payload.command, - input: payload.input, - source: payload.source, - command_actions: payload.command_actions, - }, - ); - } - } - ItemKind::ToolCall => { - if let Ok(payload) = - serde_json::from_value::( - payload.item.payload, - ) - { - let details = WorkerEvent::ToolCallDetails { - tool_use_id: payload.tool_call_id.clone(), - tool_name: payload.tool_name.clone(), - input: payload.parameters.clone(), - }; - let _ = - event_tx.send(tool_call_started_event(payload)); - let _ = event_tx.send(details); - } - } - _ => {} - } + handle_started_item(payload, event_tx); } } "item/agentMessage/delta" => { @@ -3291,6 +3230,74 @@ async fn close_btw_agent( .await; } +pub(crate) fn handle_started_item( + payload: ItemEventPayload, + event_tx: &mpsc::UnboundedSender, +) { + tracing::debug!( + item_id = %payload.item.item_id, + item_kind = ?payload.item.item_kind, + "server item started" + ); + let ItemEnvelope { + item_id, + item_kind, + payload, + } = payload.item; + match item_kind { + ItemKind::AgentMessage => { + let _ = event_tx.send(WorkerEvent::TextItemStarted { + item_id, + kind: TextItemKind::Assistant, + }); + } + ItemKind::Reasoning => { + let _ = event_tx.send(WorkerEvent::TextItemStarted { + item_id, + kind: TextItemKind::Reasoning, + }); + } + ItemKind::Plan => { + if is_proposed_plan_payload(&payload) { + let _ = event_tx.send(WorkerEvent::ProposedPlanStarted { item_id }); + } + } + ItemKind::CommandExecution => { + if let Ok(payload) = serde_json::from_value::(payload) { + let _ = event_tx.send(WorkerEvent::CommandExecutionStarted { + tool_use_id: payload.tool_call_id, + command: payload.command, + input: payload.input, + source: payload.source, + command_actions: payload.command_actions, + }); + } + } + ItemKind::ToolCall => { + if let Ok(payload) = serde_json::from_value::(payload) { + let details = WorkerEvent::ToolCallDetails { + tool_use_id: payload.tool_call_id.clone(), + tool_name: payload.tool_name.clone(), + input: payload.parameters.clone(), + }; + let _ = event_tx.send(tool_call_started_event(payload)); + let _ = event_tx.send(details); + } + } + ItemKind::ContextCompaction => { + let _ = event_tx.send(WorkerEvent::SessionCompactionStarted); + } + ItemKind::UserMessage + | ItemKind::ToolResult + | ItemKind::FileChange + | ItemKind::McpToolCall + | ItemKind::WebSearch + | ItemKind::ImageView + | ItemKind::ApprovalRequest + | ItemKind::ApprovalDecision => {} + } +} + pub(crate) fn handle_completed_item( payload: ItemEventPayload, event_tx: &mpsc::UnboundedSender, @@ -3412,6 +3419,42 @@ pub(crate) fn handle_completed_item( payload, .. } => { + let error = payload.get("error").filter(|error| !error.is_null()); + let failed = payload + .get("is_error") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false) + || payload + .get("failed") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false) + || payload + .get("status") + .and_then(serde_json::Value::as_str) + .is_some_and(|status| { + status.eq_ignore_ascii_case("failed") + || status.eq_ignore_ascii_case("error") + }) + || payload + .get("title") + .and_then(serde_json::Value::as_str) + .is_some_and(|title| title.eq_ignore_ascii_case("Compaction failed")) + || error.is_some(); + if failed { + let message = error + .and_then(|error| { + error + .as_str() + .or_else(|| error.get("message").and_then(serde_json::Value::as_str)) + }) + .or_else(|| payload.get("message").and_then(serde_json::Value::as_str)) + .map(str::trim) + .filter(|message| !message.is_empty()) + .unwrap_or("Context compaction failed") + .to_string(); + let _ = event_tx.send(WorkerEvent::SessionCompactionFailed { message }); + return; + } let title = payload .get("title") .and_then(serde_json::Value::as_str) diff --git a/crates/tui/src/worker_queue_compaction_tests.rs b/crates/tui/src/worker_queue_compaction_tests.rs index 190e72fc..3771224e 100644 --- a/crates/tui/src/worker_queue_compaction_tests.rs +++ b/crates/tui/src/worker_queue_compaction_tests.rs @@ -98,7 +98,7 @@ fn context_compaction_worker_event_adds_history_item() { let history = scrollback_plain_lines(&widget.drain_scrollback_lines(100)).join("\n"); assert!( - history.contains("Context Compaction"), + history.contains("Context compacted"), "expected completed context compaction to be visible in history:\n{history}" ); } From 1a601aa81b2c122bc2076300657956fa54c7d33b Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Wed, 15 Jul 2026 23:45:21 +0800 Subject: [PATCH 2/5] fix: simplify unified exec responses and rename process handles --- crates/core/src/tools/background_tasks.rs | 21 +-- .../core/src/tools/handlers/exec_command.rs | 173 +++++++++--------- crates/core/src/tools/registry.rs | 41 +++-- crates/core/src/tools/registry_plan.rs | 14 +- crates/core/src/tools/unified_exec/mod.rs | 4 +- crates/protocol/src/task.rs | 30 ++- .../src/runtime/turn_exec/tool_display.rs | 33 +++- crates/server/tests/unified_exec_tool_e2e.rs | 19 +- .../L2-DES-TOOL-001-built-in-tool-system.md | 2 +- 9 files changed, 200 insertions(+), 137 deletions(-) diff --git a/crates/core/src/tools/background_tasks.rs b/crates/core/src/tools/background_tasks.rs index 326321b5..2123a6e8 100644 --- a/crates/core/src/tools/background_tasks.rs +++ b/crates/core/src/tools/background_tasks.rs @@ -41,7 +41,7 @@ impl BackgroundTaskStore { pub(crate) async fn register_command( &self, owner_session_id: SessionId, - process_session_id: i32, + process_id: i32, command: String, process: Arc, ) -> TaskInfo { @@ -52,7 +52,7 @@ impl BackgroundTaskStore { state: TaskState::Running, agent: None, command: Some(CommandTaskMetadata { - process_session_id, + process_id, command, exit_code: None, }), @@ -94,14 +94,14 @@ impl BackgroundTaskStore { command.exit_code = exit_code; } state.output = Some(output); - let process_session_id = state + let process_id = state .info .command .as_ref() - .map(|command| command.process_session_id); + .map(|command| command.process_id); drop(state); - if let Some(process_session_id) = process_session_id { - self.process_store.remove(process_session_id).await; + if let Some(process_id) = process_id { + self.process_store.remove(process_id).await; } task.notify.notify_waiters(); } @@ -176,13 +176,10 @@ impl BackgroundTaskStore { state.info.state = TaskState::Canceled; state.output = None; let info = state.info.clone(); - let process_session_id = info - .command - .as_ref() - .map(|command| command.process_session_id); + let process_id = info.command.as_ref().map(|command| command.process_id); drop(state); - if let Some(process_session_id) = process_session_id { - self.process_store.remove(process_session_id).await; + if let Some(process_id) = process_id { + self.process_store.remove(process_id).await; } task.notify.notify_waiters(); Some(info) diff --git a/crates/core/src/tools/handlers/exec_command.rs b/crates/core/src/tools/handlers/exec_command.rs index 191af15b..a3a5dcf7 100644 --- a/crates/core/src/tools/handlers/exec_command.rs +++ b/crates/core/src/tools/handlers/exec_command.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use async_trait::async_trait; use devo_protocol::SessionId; +#[cfg(test)] use uuid::Uuid; use crate::apply_patch::exec_apply_patch; @@ -179,7 +180,7 @@ impl ToolHandler for ExecCommandHandler { ToolContent::Json(json) => (json.to_string(), Some(json)), ToolContent::Mixed { text, json } => (text.unwrap_or_default(), json), }; - let content = format_apply_patch_intercept_response(&text); + let content = text; return if output.is_error { Ok(ToolResult::error( ToolResultContent::Text(content.clone()), @@ -202,7 +203,7 @@ impl ToolHandler for ExecCommandHandler { }; } - let Some(session_id) = self.store.reserve_process_id().await else { + let Some(process_id) = self.store.reserve_process_id().await else { return Ok(ToolResult::error( ToolResultContent::Text(format!( "max unified exec processes ({}) reached; cannot allocate process", @@ -217,7 +218,7 @@ impl ToolHandler for ExecCommandHandler { }; let spawned_process = UnifiedExecProcess::spawn( - session_id, + process_id, &args.cmd, &cwd, args.shell.as_deref(), @@ -228,7 +229,7 @@ impl ToolHandler for ExecCommandHandler { let (proc, _broadcast_rx) = match spawned_process { Ok(spawned) => spawned, Err(error) => { - self.store.release_reserved(session_id).await; + self.store.release_reserved(process_id).await; return Err(ToolCallError::ExecutionFailed(format!( "failed to spawn process: {error}" ))); @@ -237,7 +238,7 @@ impl ToolHandler for ExecCommandHandler { let proc = Arc::new(proc); self.store - .insert_reserved(session_id, Arc::clone(&proc)) + .insert_reserved(process_id, Arc::clone(&proc)) .await; if execution_mode == ExecExecutionMode::Background { @@ -247,7 +248,7 @@ impl ToolHandler for ExecCommandHandler { })?; let task = self .background_tasks - .register_command(owner_session_id, session_id, args.cmd, Arc::clone(&proc)) + .register_command(owner_session_id, process_id, args.cmd, Arc::clone(&proc)) .await; let task_id = task.task_id.clone(); let background_tasks = Arc::clone(&self.background_tasks); @@ -257,12 +258,8 @@ impl ToolHandler for ExecCommandHandler { } let mut rx = proc.subscribe(); let output = collect_output(&mut rx, &proc, 100, args.max_output_tokens).await; - let response = format_exec_response( - &output, - Some(session_id), - Some(generate_chunk_id()), - /*warning*/ None, - ); + let response = + format_exec_response(&output, Some(process_id), /*warning*/ None); background_tasks .complete_command(&task_id, output.exit_code, response) .await; @@ -288,7 +285,7 @@ impl ToolHandler for ExecCommandHandler { let cancel_task = tokio::spawn(async move { cancel_token.cancelled().await; proc_for_cancel.terminate(); - store_for_cancel.remove(session_id).await; + store_for_cancel.remove(process_id).await; }); let mut rx = proc.subscribe(); @@ -301,26 +298,21 @@ impl ToolHandler for ExecCommandHandler { ) => output, _ = ctx.cancel_token.cancelled() => { proc.terminate(); - self.store.remove(session_id).await; + self.store.remove(process_id).await; cancel_task.abort(); return Err(ToolCallError::Cancelled); } }; cancel_task.abort(); let warning = if output.exit_code.is_some() { - self.store.remove(session_id).await; + self.store.remove(process_id).await; None } else { let process_count = self.store.len().await; (process_count >= WARNING_PROCESSES).then(|| open_process_warning(process_count)) }; - let response = format_exec_response( - &output, - Some(session_id), - Some(generate_chunk_id()), - warning.as_deref(), - ); + let response = format_exec_response(&output, Some(process_id), warning.as_deref()); Ok(ToolResult::success( ToolResultContent::Text(response), "Command executed", @@ -349,15 +341,15 @@ impl WriteStdinHandler { input_schema: JsonSchema::object( std::collections::BTreeMap::from([ ( - "session_id".to_string(), - JsonSchema::integer(Some("Process session ID")), + "process_id".to_string(), + JsonSchema::integer(Some("Running process ID")), ), ( "chars".to_string(), JsonSchema::string(Some("Characters to write to stdin")), ), ]), - Some(vec!["session_id".to_string(), "chars".to_string()]), + Some(vec!["process_id".to_string(), "chars".to_string()]), None, ), output_mode: ToolOutputMode::Mixed, @@ -386,9 +378,11 @@ impl ToolHandler for WriteStdinHandler { _progress: Option, ) -> Result { let args = WriteStdinArgs { - session_id: input["session_id"] - .as_i64() - .ok_or_else(|| ToolCallError::InvalidInput("missing 'session_id' field".into()))? + process_id: input + .get("process_id") + .or_else(|| input.get("session_id")) + .and_then(serde_json::Value::as_i64) + .ok_or_else(|| ToolCallError::InvalidInput("missing 'process_id' field".into()))? as i32, chars: input["chars"].as_str().unwrap_or("").to_string(), yield_time_ms: input["yield_time_ms"] @@ -400,8 +394,8 @@ impl ToolHandler for WriteStdinHandler { .unwrap_or(crate::unified_exec::MAX_OUTPUT_TOKENS), }; - let proc = self.store.get(args.session_id).await.ok_or_else(|| { - ToolCallError::ExecutionFailed(format!("Unknown process id {}", args.session_id)) + let proc = self.store.get(args.process_id).await.ok_or_else(|| { + ToolCallError::ExecutionFailed(format!("Unknown process id {}", args.process_id)) })?; if !args.chars.is_empty() { @@ -432,15 +426,10 @@ impl ToolHandler for WriteStdinHandler { .await; if output.exit_code.is_some() { - self.store.remove(args.session_id).await; + self.store.remove(args.process_id).await; } - let response = format_exec_response( - &output, - Some(args.session_id), - Some(generate_chunk_id()), - /*warning*/ None, - ); + let response = format_exec_response(&output, Some(args.process_id), /*warning*/ None); Ok(ToolResult::success( ToolResultContent::Text(response), "Input written", @@ -450,56 +439,35 @@ impl ToolHandler for WriteStdinHandler { fn format_exec_response( output: &ProcessOutput, - session_id: Option, - chunk_id: Option, + process_id: Option, warning: Option<&str>, ) -> String { let mut parts = Vec::new(); - if let Some(chunk_id) = chunk_id - && !chunk_id.is_empty() - { - parts.push(format!("Chunk ID: {chunk_id}")); - } - - parts.push(format!("Wall time: {:.4} seconds", output.wall_time_secs)); - if let Some(code) = output.exit_code { parts.push(format!("Process exited with code {code}")); } - if let Some(sid) = session_id + if let Some(process_id) = process_id && output.exit_code.is_none() { - parts.push(format!("Process running with session ID {sid}")); + parts.push(format!("Process running with process ID {process_id}")); } if let Some(warning) = warning { parts.push(warning.to_string()); } - - parts.push(format!( - "Original token count: {}", - output.original_token_count - )); - parts.push("Output:".to_string()); - parts.push(output.output.clone()); + if !output.output.is_empty() { + parts.push(output.output.clone()); + } parts.join("\n") } -fn generate_chunk_id() -> String { - Uuid::new_v4().to_string().chars().take(6).collect() -} - fn open_process_warning(process_count: usize) -> String { format!( "Warning: The maximum number of unified exec processes you can keep open is {WARNING_PROCESSES} and you currently have {process_count} processes open. Reuse older processes or close them to prevent automatic pruning of old processes" ) } -fn format_apply_patch_intercept_response(content: &str) -> String { - format!("Wall time: 0.0000 seconds\nOutput:\n{content}") -} - #[allow(dead_code)] fn progress_delta_chunks(bytes: &[u8]) -> Vec { let text = String::from_utf8_lossy(bytes); @@ -663,12 +631,8 @@ mod tests { truncated: false, original_token_count: 3, }; - let text = format_exec_response(&output, None, None, /*warning*/ None); - assert!(text.contains("Wall time: 1.5000")); - assert!(text.contains("Process exited with code 0")); - assert!(text.contains("hello world")); - assert!(!text.contains("session ID")); - assert!(text.contains("Original token count: 3")); + let text = format_exec_response(&output, None, /*warning*/ None); + assert_eq!(text, "Process exited with code 0\nhello world"); } #[test] @@ -680,9 +644,8 @@ mod tests { truncated: false, original_token_count: 3, }; - let text = format_exec_response(&output, Some(42), None, /*warning*/ None); - assert!(text.contains("Process running with session ID 42")); - assert!(!text.contains("exit code")); + let text = format_exec_response(&output, Some(42), /*warning*/ None); + assert_eq!(text, "Process running with process ID 42\nbuilding..."); } #[test] @@ -694,12 +657,12 @@ mod tests { truncated: true, original_token_count: 3, }; - let text = format_exec_response(&output, Some(1), None, /*warning*/ None); - assert!(text.contains("Output:")); + let text = format_exec_response(&output, Some(1), /*warning*/ None); + assert_eq!(text, "Process running with process ID 1\nlong output..."); } #[test] - fn format_exec_response_with_both_exit_and_session() { + fn format_exec_response_with_both_exit_and_process_id() { let output = ProcessOutput { output: "done".into(), exit_code: Some(0), @@ -707,10 +670,8 @@ mod tests { truncated: false, original_token_count: 1, }; - // When exit_code is Some, session_id is not shown even if provided - let text = format_exec_response(&output, Some(99), None, /*warning*/ None); - assert!(text.contains("Process exited with code 0")); - assert!(!text.contains("session ID")); + let text = format_exec_response(&output, Some(99), /*warning*/ None); + assert_eq!(text, "Process exited with code 0\ndone"); } #[test] @@ -726,12 +687,56 @@ mod tests { let text = format_exec_response( &output, Some(42), - None, Some(&open_process_warning(WARNING_PROCESSES)), ); - assert!(text.contains("currently have 60 processes open")); - assert!(text.contains("Reuse older processes")); + assert_eq!( + text, + format!( + "Process running with process ID 42\n{}\nbuilding...", + open_process_warning(WARNING_PROCESSES) + ) + ); + } + + #[tokio::test] + async fn write_stdin_accepts_legacy_session_id() { + let handler = WriteStdinHandler::new(Arc::new(ProcessStore::new())); + let error = handler + .handle( + test_ctx(std::env::temp_dir()), + serde_json::json!({ "session_id": 42, "chars": "" }), + /*_progress*/ None, + ) + .await + .expect_err("unknown legacy process id"); + let ToolCallError::ExecutionFailed(message) = error else { + panic!("legacy session_id should be parsed as a process id"); + }; + + assert_eq!(message, "Unknown process id 42"); + } + + #[tokio::test] + async fn write_stdin_prefers_process_id_over_legacy_session_id() { + let handler = WriteStdinHandler::new(Arc::new(ProcessStore::new())); + let error = handler + .handle( + test_ctx(std::env::temp_dir()), + serde_json::json!({ + "process_id": 42, + "session_id": 99, + "chars": "" + }), + /*_progress*/ None, + ) + .await + .expect_err("unknown process id"); + let ToolCallError::ExecutionFailed(message) = error else { + panic!("process_id should take precedence over legacy session_id"); + }; + + assert_eq!(message, "Unknown process id 42"); } #[test] @@ -889,7 +894,7 @@ mod tests { .expect("handle exec command"); let (text, metadata) = result_text_and_metadata(&output.content); - assert!(text.starts_with("Wall time: 0.0000 seconds\nOutput:\n")); + assert!(text.starts_with("Success. Updated the following files:")); assert!(text.contains("Success. Updated the following files:")); assert!(!text.contains("\"diagnostics\"")); assert_eq!( @@ -924,7 +929,7 @@ mod tests { .expect("handle exec command"); let (text, metadata) = result_text_and_metadata(&output.content); - assert!(text.starts_with("Wall time: 0.0000 seconds\nOutput:\n")); + assert!(text.starts_with("Success. Updated the following files:")); assert!(text.contains("Success. Updated the following files:")); assert!(!text.contains("\"diagnostics\"")); assert_eq!( diff --git a/crates/core/src/tools/registry.rs b/crates/core/src/tools/registry.rs index 9aab15d2..5b28703b 100644 --- a/crates/core/src/tools/registry.rs +++ b/crates/core/src/tools/registry.rs @@ -307,32 +307,20 @@ fn unified_exec_output_schema(tool_name: &str) -> Option { Some(serde_json::json!({ "type": "object", "properties": { - "chunk_id": { - "type": "string", - "description": "Chunk identifier included when the response reports one." - }, - "wall_time_seconds": { - "type": "number", - "description": "Elapsed wall time spent waiting for output in seconds." - }, "exit_code": { "type": "number", "description": "Process exit code when the command finished during this call." }, - "session_id": { + "process_id": { "type": "number", - "description": "Session identifier to pass to write_stdin when the process is still running." - }, - "original_token_count": { - "type": "number", - "description": "Approximate token count before output truncation." + "description": "Process identifier to pass to write_stdin when the process is still running." }, "output": { "type": "string", "description": "Command output text, possibly truncated." } }, - "required": ["wall_time_seconds", "output"], + "required": ["output"], "additionalProperties": false })) } @@ -777,7 +765,28 @@ mod tests { let registry = builder.build(); let defs = registry.tool_definitions(); - assert!(defs[0].output_schema.is_some()); + assert_eq!( + defs[0].output_schema, + Some(serde_json::json!({ + "type": "object", + "properties": { + "exit_code": { + "type": "number", + "description": "Process exit code when the command finished during this call." + }, + "process_id": { + "type": "number", + "description": "Process identifier to pass to write_stdin when the process is still running." + }, + "output": { + "type": "string", + "description": "Command output text, possibly truncated." + } + }, + "required": ["output"], + "additionalProperties": false + })) + ); } #[test] diff --git a/crates/core/src/tools/registry_plan.rs b/crates/core/src/tools/registry_plan.rs index 1aa6a81f..1df066a1 100644 --- a/crates/core/src/tools/registry_plan.rs +++ b/crates/core/src/tools/registry_plan.rs @@ -623,7 +623,7 @@ fn exec_command_schema() -> JsonSchema { ( "execution_mode".to_string(), JsonSchema::string(Some( - "attached (default) returns output or a process session; background returns a task id immediately.", + "attached (default) returns output or a process ID; background returns a task id immediately.", )), ), ( @@ -646,8 +646,8 @@ fn write_stdin_schema() -> JsonSchema { JsonSchema::object( BTreeMap::from([ ( - "session_id".to_string(), - JsonSchema::integer(Some("Session ID of the running exec_command process")), + "process_id".to_string(), + JsonSchema::integer(Some("Process ID of the running exec_command process")), ), ( "chars".to_string(), @@ -666,7 +666,7 @@ fn write_stdin_schema() -> JsonSchema { JsonSchema::number(Some("Maximum number of tokens of output to return.")), ), ]), - Some(vec!["session_id".to_string()]), + Some(vec!["process_id".to_string()]), Some(false), ) } @@ -941,7 +941,7 @@ pub fn build_tool_registry_plan(config: &ToolPlanConfig) -> ToolRegistryPlan { ToolSpec { name: "exec_command".to_string(), description: - "Run a shell command in attached or background mode. Attached mode returns output or a process session for write_stdin; background mode returns a task id for await_task, list_tasks, and cancel_task." + "Run a shell command in attached or background mode. Attached mode returns output or a process ID for write_stdin; background mode returns a task id for await_task, list_tasks, and cancel_task." .to_string(), input_schema: exec_command_schema(), output_mode: ToolOutputMode::Mixed, @@ -1042,10 +1042,10 @@ mod tests { } #[test] - fn schema_write_stdin_requires_session_id() { + fn schema_write_stdin_requires_process_id() { let schema = write_stdin_schema(); let required = schema.required.as_ref().unwrap(); - assert!(required.contains(&"session_id".to_string())); + assert_eq!(required, &vec!["process_id".to_string()]); } #[test] diff --git a/crates/core/src/tools/unified_exec/mod.rs b/crates/core/src/tools/unified_exec/mod.rs index d6ac6e90..35a6a66a 100644 --- a/crates/core/src/tools/unified_exec/mod.rs +++ b/crates/core/src/tools/unified_exec/mod.rs @@ -1,6 +1,6 @@ //! Shared types and limits for long-lived command execution tools. //! -//! The process runner, bounded output buffer, and session store live below this +//! The process runner, bounded output buffer, and process store live below this //! module so tool handlers can enforce one set of limits for exec and stdin //! polling without duplicating lifecycle policy. @@ -42,7 +42,7 @@ pub struct ExecCommandArgs { } pub struct WriteStdinArgs { - pub session_id: i32, + pub process_id: i32, pub chars: String, pub yield_time_ms: u64, pub max_output_tokens: usize, diff --git a/crates/protocol/src/task.rs b/crates/protocol/src/task.rs index 18f01200..cc3ffe4b 100644 --- a/crates/protocol/src/task.rs +++ b/crates/protocol/src/task.rs @@ -51,7 +51,8 @@ pub struct AgentTaskMetadata { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, TS)] pub struct CommandTaskMetadata { - pub process_session_id: i32, + #[serde(alias = "process_session_id")] + pub process_id: i32, pub command: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub exit_code: Option, @@ -146,4 +147,31 @@ mod tests { assert_eq!(restored, result); } + + #[test] + fn command_metadata_reads_legacy_process_session_id_and_writes_process_id() { + let restored: CommandTaskMetadata = serde_json::from_value(serde_json::json!({ + "process_session_id": 42, + "command": "cargo test", + "exit_code": 0 + })) + .expect("deserialize legacy command metadata"); + + assert_eq!( + restored, + CommandTaskMetadata { + process_id: 42, + command: "cargo test".to_string(), + exit_code: Some(0), + } + ); + assert_eq!( + serde_json::to_value(restored).expect("serialize command metadata"), + serde_json::json!({ + "process_id": 42, + "command": "cargo test", + "exit_code": 0 + }) + ); + } } diff --git a/crates/server/src/runtime/turn_exec/tool_display.rs b/crates/server/src/runtime/turn_exec/tool_display.rs index a3a1ac2a..81029b77 100644 --- a/crates/server/src/runtime/turn_exec/tool_display.rs +++ b/crates/server/src/runtime/turn_exec/tool_display.rs @@ -139,8 +139,9 @@ pub(super) fn command_display_from_input(tool_name: &str, input: &serde_json::Va .unwrap_or_default() .to_string(), "write_stdin" => { - let session_id = input - .get("session_id") + let process_id = input + .get("process_id") + .or_else(|| input.get("session_id")) .and_then(serde_json::Value::as_i64) .map(|id| id.to_string()) .unwrap_or_else(|| "?".to_string()); @@ -149,9 +150,9 @@ pub(super) fn command_display_from_input(tool_name: &str, input: &serde_json::Va .and_then(serde_json::Value::as_str) .unwrap_or_default(); if chars.is_empty() { - format!("poll session {session_id}") + format!("poll process {process_id}") } else { - format!("write_stdin session {session_id}") + format!("write_stdin process {process_id}") } } "read" => { @@ -333,7 +334,7 @@ mod tests { use devo_protocol::SessionHistoryMetadata; use pretty_assertions::assert_eq; - use super::command_actions_from_tool_input; + use super::{command_actions_from_tool_input, command_display_from_input}; use crate::projection::history_item_from_turn_item; #[test] @@ -398,4 +399,26 @@ mod tests { ); } } + + #[test] + fn write_stdin_display_prefers_process_id_and_reads_legacy_session_id() { + assert_eq!( + command_display_from_input( + "write_stdin", + &serde_json::json!({ + "process_id": 42, + "session_id": 99, + "chars": "hello" + }), + ), + "write_stdin process 42" + ); + assert_eq!( + command_display_from_input( + "write_stdin", + &serde_json::json!({ "session_id": 99, "chars": "" }), + ), + "poll process 99" + ); + } } diff --git a/crates/server/tests/unified_exec_tool_e2e.rs b/crates/server/tests/unified_exec_tool_e2e.rs index a977e51e..d4b669b7 100644 --- a/crates/server/tests/unified_exec_tool_e2e.rs +++ b/crates/server/tests/unified_exec_tool_e2e.rs @@ -89,12 +89,12 @@ impl ModelProviderSDK for InteractiveExecProvider { }), ), 1 => { - let session_id = extract_process_session_id(&request)?; + let process_id = extract_process_id(&request)?; tool_call_events( "stdin-1", "write_stdin", serde_json::json!({ - "session_id": session_id, + "process_id": process_id, "chars": "hello\n", "yield_time_ms": 5000, "max_output_tokens": 1000 @@ -209,7 +209,7 @@ fn interactive_command() -> &'static str { "Write-Output 'ready'; $line = Read-Host; Write-Output \"received:$line\"; Start-Sleep -Milliseconds 100; Write-Output 'done'" } -fn extract_process_session_id(request: &ModelRequest) -> Result { +fn extract_process_id(request: &ModelRequest) -> Result { let content = request .messages .iter() @@ -223,12 +223,12 @@ fn extract_process_session_id(request: &ModelRequest) -> Result { _ => None, }) .context("exec_command tool result")?; - let marker = "Process running with session ID "; - let session_id = content + let marker = "Process running with process ID "; + let process_id = content .lines() .find_map(|line| line.strip_prefix(marker)) - .context("running process session id")?; - session_id.parse().context("parse process session id") + .context("running process id")?; + process_id.parse().context("parse process id") } fn extract_background_task_id(request: &ModelRequest) -> Result { @@ -303,7 +303,7 @@ async fn long_running_exec_command_accepts_write_stdin_and_exits() -> Result<()> let requests = provider.requests(); assert_eq!(requests.len(), 3); let exec_result = tool_result(&requests[1], "exec-1").context("exec result")?; - assert!(exec_result.contains("Process running with session ID")); + assert!(exec_result.contains("Process running with process ID")); assert!(exec_result.contains("ready")); let stdin_result = tool_result(&requests[2], "stdin-1").context("stdin result")?; assert!(stdin_result.contains("received:hello")); @@ -323,7 +323,8 @@ async fn background_exec_lists_and_awaits_terminal_command_task() -> Result<()> let listed = tool_result(&requests[2], "list-tasks-1").context("list task result")?; assert!(listed.contains("\"kind\":\"command\"")); assert!(listed.contains("\"state\":\"running\"")); - assert!(listed.contains("process_session_id")); + assert!(listed.contains("process_id")); + assert!(!listed.contains("process_session_id")); let awaited = tool_result(&requests[3], "await-task-1").context("await task result")?; assert!(awaited.contains("\"outcome\":\"terminal\"")); assert!(awaited.contains("\"state\":\"completed\"")); diff --git a/specs/L2/tool/L2-DES-TOOL-001-built-in-tool-system.md b/specs/L2/tool/L2-DES-TOOL-001-built-in-tool-system.md index 80e23878..34f50e47 100644 --- a/specs/L2/tool/L2-DES-TOOL-001-built-in-tool-system.md +++ b/specs/L2/tool/L2-DES-TOOL-001-built-in-tool-system.md @@ -331,7 +331,7 @@ Output must be bounded so noisy commands, large files, web content, or backgroun Command tools may start processes that continue after the originating tool call returns. Such processes should be registered with the tool supervisor and exposed through `L2-DES-AGENT-002` and `L2-DES-APP-003`. -`exec_command.execution_mode` accepts `attached` (default) or `background`. Attached execution preserves the interactive `session_id` and `write_stdin` contract. Background execution returns a `TaskId` immediately and registers a command task that is visible through the shared `list_tasks`, `await_task`, and `cancel_task` tools. Command tasks use the same public states as agent tasks: `waiting_approval`, `running`, `completed`, `failed`, and `canceled`. +`exec_command.execution_mode` accepts `attached` (default) or `background`. Attached execution returns a short-lived `process_id` for subsequent `write_stdin` calls; this process handle is distinct from the conversation `session_id`. `write_stdin` continues to accept the legacy `session_id` input for compatibility, but new tool definitions advertise only `process_id`. Background execution returns a `TaskId` immediately and registers a command task that is visible through the shared `list_tasks`, `await_task`, and `cancel_task` tools. Command tasks use the same public states as agent tasks: `waiting_approval`, `running`, `completed`, `failed`, and `canceled`. `await_task` returns background command output only after terminal completion; a deadline expiry returns task metadata without partial command output. `cancel_task` terminates the process, removes it from the live process store, and retains canceled task metadata for subsequent inspection. From c306d81dc3fe155fc815ef883f60a554bc262d90 Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Thu, 16 Jul 2026 00:15:24 +0800 Subject: [PATCH 3/5] fix: compile, update models.json, remove "no output". --- crates/cli/src/prompt_command.rs | 59 ++++++++++++++++++- crates/core/models.json | 4 +- crates/core/src/tools/bash.txt | 3 +- .../core/src/tools/client_terminal_shell.rs | 36 ++++++++++- crates/core/src/tools/handlers/bash.rs | 4 +- .../core/src/tools/handlers/exec_command.rs | 6 +- .../core/src/tools/handlers/shell_command.rs | 6 +- crates/core/src/tools/registry_plan.rs | 38 +++++++++++- crates/core/src/tools/shell_exec.rs | 8 +-- crates/tui/src/exec_cell/render.rs | 40 ++++++++++--- 10 files changed, 175 insertions(+), 29 deletions(-) diff --git a/crates/cli/src/prompt_command.rs b/crates/cli/src/prompt_command.rs index ce46db9b..6f5d53d3 100644 --- a/crates/cli/src/prompt_command.rs +++ b/crates/cli/src/prompt_command.rs @@ -364,6 +364,15 @@ enum PromptJsonlEvent<'a> { phase: &'static str, message: &'a str, }, + #[serde(rename = "session.compaction.started")] + ContextCompactionStarted { session_id: &'a str }, + #[serde(rename = "session.compaction.completed")] + ContextCompactionCompleted { session_id: &'a str }, + #[serde(rename = "session.compaction.failed")] + ContextCompactionFailed { + session_id: &'a str, + message: &'a str, + }, #[serde(rename = "turn.usage_delta")] UsageDelta { session_id: &'a str, @@ -473,6 +482,18 @@ fn write_query_event_jsonl(session_id: &str, event: &QueryEvent) -> Result<()> { message: status.message.as_str(), }) } + QueryEvent::ContextCompactionStarted => { + write_jsonl(&PromptJsonlEvent::ContextCompactionStarted { session_id }) + } + QueryEvent::ContextCompactionCompleted => { + write_jsonl(&PromptJsonlEvent::ContextCompactionCompleted { session_id }) + } + QueryEvent::ContextCompactionFailed { message } => { + write_jsonl(&PromptJsonlEvent::ContextCompactionFailed { + session_id, + message, + }) + } QueryEvent::UsageDelta { usage } => write_jsonl(&PromptJsonlEvent::UsageDelta { session_id, usage: PromptUsageDelta::new(usage), @@ -586,7 +607,7 @@ mod tests { use pretty_assertions::assert_eq; use super::latest_assistant_text; - use super::{PromptResult, PromptUsage}; + use super::{PromptJsonlEvent, PromptResult, PromptUsage}; use devo_core::ContentBlock; use devo_core::Message; use devo_core::Role; @@ -628,6 +649,42 @@ mod tests { ); } + #[test] + fn prompt_jsonl_compaction_lifecycle_uses_session_event_shapes() { + assert_eq!( + serde_json::to_value(PromptJsonlEvent::ContextCompactionStarted { + session_id: "session-1", + }) + .expect("serialize compaction started"), + serde_json::json!({ + "type": "session.compaction.started", + "session_id": "session-1" + }) + ); + assert_eq!( + serde_json::to_value(PromptJsonlEvent::ContextCompactionCompleted { + session_id: "session-1", + }) + .expect("serialize compaction completed"), + serde_json::json!({ + "type": "session.compaction.completed", + "session_id": "session-1" + }) + ); + assert_eq!( + serde_json::to_value(PromptJsonlEvent::ContextCompactionFailed { + session_id: "session-1", + message: "compaction provider failed", + }) + .expect("serialize compaction failed"), + serde_json::json!({ + "type": "session.compaction.failed", + "session_id": "session-1", + "message": "compaction provider failed" + }) + ); + } + #[test] fn latest_assistant_text_returns_none_for_empty_messages() { assert_eq!(latest_assistant_text(&[]), None); diff --git a/crates/core/models.json b/crates/core/models.json index e82ecd3b..9dedfbc9 100644 --- a/crates/core/models.json +++ b/crates/core/models.json @@ -201,8 +201,8 @@ "high", "max" ], - "context_window": 1000000, - "max_tokens": 384000, + "context_window": 1048565, + "max_tokens": 8096, "effective_context_window_percent": 95, "truncation_policy": { "mode": "tokens", diff --git a/crates/core/src/tools/bash.txt b/crates/core/src/tools/bash.txt index 590cfde8..959888b0 100644 --- a/crates/core/src/tools/bash.txt +++ b/crates/core/src/tools/bash.txt @@ -16,6 +16,7 @@ Before executing the command, please follow these steps: - For example, before running "mkdir foo/bar", first use `ls foo` to check that "foo" exists and is the intended parent directory 2. Command Execution: + - Start the command string with a brief `# ...` comment on its own line explaining the command's purpose, then put the command on the next line - Always quote file paths that contain spaces with double quotes (e.g., rm "path with spaces/file.txt") - Examples of proper quoting: - mkdir "/Users/name/My Documents" (correct) @@ -43,7 +44,7 @@ Usage notes: - If the commands are independent and can run in parallel, make multiple shell_command tool calls in a single message. For example, if you need to run "git status" and "git diff", send a single message with two shell_command tool calls in parallel. - ${chaining} - Use ';' only when you need to run commands sequentially but don't care if earlier commands fail - - DO NOT use newlines to separate commands (newlines are ok in quoted strings) + - DO NOT use bare newlines to separate multiple commands. The leading purpose-comment newline and newlines inside quoted strings or scripts are allowed - AVOID using `cd && `. Use the `workdir` parameter to change directories instead. Use workdir="/foo/bar" with command: pytest tests diff --git a/crates/core/src/tools/client_terminal_shell.rs b/crates/core/src/tools/client_terminal_shell.rs index 4eb5bfca..34e44fd1 100644 --- a/crates/core/src/tools/client_terminal_shell.rs +++ b/crates/core/src/tools/client_terminal_shell.rs @@ -216,7 +216,7 @@ fn terminal_result_metadata( } ], "terminalId": terminal_id, - "output": if output_text.is_empty() { "(no output)" } else { output_text.as_str() }, + "output": output_text, "truncated": truncated, "exitStatus": exit_status, "command": preview(&request.command), @@ -473,6 +473,40 @@ mod tests { } } + #[test] + fn terminal_metadata_preserves_empty_output() { + let request = request(); + let metadata = terminal_result_metadata( + "term_1", + &request, + Some(ClientTerminalOutput { + output: String::new(), + truncated: false, + exit_status: Some(ok_status()), + }), + ); + + assert_eq!( + metadata, + serde_json::json!({ + "content": [{ + "type": "terminal", + "terminalId": "term_1" + }], + "terminalId": "term_1", + "output": "", + "truncated": false, + "exitStatus": { + "exitCode": 0, + "signal": null + }, + "command": "echo hi", + "description": "test command", + "cwd": request.workdir + }) + ); + } + #[tokio::test] async fn client_terminal_success_returns_terminal_content_and_releases() { let terminal = fake_terminal( diff --git a/crates/core/src/tools/handlers/bash.rs b/crates/core/src/tools/handlers/bash.rs index e95d522a..18fa1c13 100644 --- a/crates/core/src/tools/handlers/bash.rs +++ b/crates/core/src/tools/handlers/bash.rs @@ -29,10 +29,10 @@ impl BashHandler { Self { spec: ToolSpec { name: "shell_command".into(), - description: "Executes a shell command in the selected platform shell with optional timeout and output limits.".into(), + description: "Executes a shell command in the selected platform shell with optional timeout and output limits. Start the command string with a brief `# ...` comment on its own line explaining the command's purpose, then put the command on the next line.".into(), input_schema: JsonSchema::object( std::collections::BTreeMap::from([ - ("command".to_string(), JsonSchema::string(Some("The command to execute"))), + ("command".to_string(), JsonSchema::string(Some("The command to execute. Start with a brief `# ...` purpose comment on the line above the command."))), ("cmd".to_string(), JsonSchema::string(Some("Alias for command"))), ("timeout".to_string(), JsonSchema::integer(Some("Optional timeout in milliseconds"))), ("timeout_ms".to_string(), JsonSchema::integer(Some("Alias for timeout"))), diff --git a/crates/core/src/tools/handlers/exec_command.rs b/crates/core/src/tools/handlers/exec_command.rs index a3a5dcf7..f48736b8 100644 --- a/crates/core/src/tools/handlers/exec_command.rs +++ b/crates/core/src/tools/handlers/exec_command.rs @@ -48,12 +48,14 @@ impl ExecCommandHandler { background_tasks, spec: ToolSpec { name: "exec_command".into(), - description: "Execute a command with PTY support and process management.".into(), + description: "Execute a command with PTY support and process management. Start the command string with a brief `# ...` comment on its own line explaining the command's purpose, then put the command on the next line.".into(), input_schema: JsonSchema::object( std::collections::BTreeMap::from([ ( "cmd".to_string(), - JsonSchema::string(Some("The command to execute")), + JsonSchema::string(Some( + "The command to execute. Start with a brief `# ...` purpose comment on the line above the command.", + )), ), ( "workdir".to_string(), diff --git a/crates/core/src/tools/handlers/shell_command.rs b/crates/core/src/tools/handlers/shell_command.rs index 1c37bbdc..4424b38f 100644 --- a/crates/core/src/tools/handlers/shell_command.rs +++ b/crates/core/src/tools/handlers/shell_command.rs @@ -31,12 +31,14 @@ impl ShellCommandHandler { Self { spec: ToolSpec { name: "shell_command".into(), - description: "Executes a shell command with optional timeout.".into(), + description: "Executes a shell command with optional timeout. Start the command string with a brief `# ...` comment on its own line explaining the command's purpose, then put the command on the next line.".into(), input_schema: JsonSchema::object( std::collections::BTreeMap::from([ ( "command".to_string(), - JsonSchema::string(Some("The command to execute")), + JsonSchema::string(Some( + "The command to execute. Start with a brief `# ...` purpose comment on the line above the command.", + )), ), ( "workdir".to_string(), diff --git a/crates/core/src/tools/registry_plan.rs b/crates/core/src/tools/registry_plan.rs index 1df066a1..34bb6133 100644 --- a/crates/core/src/tools/registry_plan.rs +++ b/crates/core/src/tools/registry_plan.rs @@ -96,7 +96,7 @@ fn shell_command_schema() -> JsonSchema { ( "command".to_string(), JsonSchema::string(Some( - "The shell command to execute in the selected platform shell", + "The shell command to execute in the selected platform shell. Start with a brief `# ...` purpose comment on the line above the command.", )), ), ( @@ -592,7 +592,9 @@ fn exec_command_schema() -> JsonSchema { BTreeMap::from([ ( "cmd".to_string(), - JsonSchema::string(Some("Shell command to execute")), + JsonSchema::string(Some( + "Shell command to execute. Start with a brief `# ...` purpose comment on the line above the command.", + )), ), ( "command".to_string(), @@ -941,7 +943,7 @@ pub fn build_tool_registry_plan(config: &ToolPlanConfig) -> ToolRegistryPlan { ToolSpec { name: "exec_command".to_string(), description: - "Run a shell command in attached or background mode. Attached mode returns output or a process ID for write_stdin; background mode returns a task id for await_task, list_tasks, and cancel_task." + "Run a shell command in attached or background mode. Start the command string with a brief `# ...` comment on its own line explaining the command's purpose, then put the command on the next line. Attached mode returns output or a process ID for write_stdin; background mode returns a task id for await_task, list_tasks, and cancel_task." .to_string(), input_schema: exec_command_schema(), output_mode: ToolOutputMode::Mixed, @@ -1063,6 +1065,36 @@ mod tests { assert!(props.contains_key("cmd")); assert!(props.contains_key("timeout_ms")); assert!(props.contains_key("tty")); + assert!( + props["command"] + .description + .as_deref() + .is_some_and(|description| description.contains("purpose comment")) + ); + } + + #[test] + fn command_tool_descriptions_request_a_leading_purpose_comment() { + let plan = build_tool_registry_plan(&ToolPlanConfig::default()); + let descriptions = plan + .specs + .iter() + .filter(|spec| matches!(spec.name.as_str(), "shell_command" | "exec_command")) + .map(|spec| (spec.name.as_str(), spec.description.as_str())) + .collect::>(); + + assert_eq!(descriptions.len(), 2); + assert!( + descriptions + .values() + .all(|description| description.contains("brief `# ...` comment")) + ); + assert!( + exec_command_schema().properties.as_ref().unwrap()["cmd"] + .description + .as_deref() + .is_some_and(|description| description.contains("purpose comment")) + ); } #[test] diff --git a/crates/core/src/tools/shell_exec.rs b/crates/core/src/tools/shell_exec.rs index a4337622..41ec22f2 100644 --- a/crates/core/src/tools/shell_exec.rs +++ b/crates/core/src/tools/shell_exec.rs @@ -319,11 +319,7 @@ pub(crate) fn merge_streams(stdout: &str, stderr: &str) -> String { result.push_str("[stderr]\n"); result.push_str(stderr); } - if result.is_empty() { - "(no output)".to_string() - } else { - result - } + result } fn platform_shell(login: bool) -> ShellSpec { @@ -791,6 +787,6 @@ mod tests { #[test] fn merge_streams_no_output() { - assert_eq!(merge_streams("", ""), "(no output)"); + assert_eq!(merge_streams("", ""), ""); } } diff --git a/crates/tui/src/exec_cell/render.rs b/crates/tui/src/exec_cell/render.rs index 5e9efa16..aeb05c49 100644 --- a/crates/tui/src/exec_cell/render.rs +++ b/crates/tui/src/exec_cell/render.rs @@ -596,15 +596,7 @@ impl ExecCell { layout.output_max_lines }; - if raw_output.lines.is_empty() { - if !call.is_unified_exec_interaction() { - lines.extend(prefix_lines( - vec![Line::from("(no output)".dim())], - Span::from(layout.output_block.initial_prefix).dim(), - Span::from(layout.output_block.subsequent_prefix), - )); - } - } else { + if !raw_output.lines.is_empty() { // Wrap first so that truncation is applied to on-screen lines // rather than logical lines. This ensures that a small number // of very long lines cannot flood the viewport. @@ -969,6 +961,36 @@ mod tests { ); } + #[test] + fn completed_command_with_empty_output_renders_only_the_command() { + let call = ExecCall { + call_id: "call-id".to_string(), + command: vec!["bash".into(), "-lc".into(), "echo hi".into()], + parsed: Vec::new(), + output: Some(CommandOutput { + exit_code: 0, + aggregated_output: String::new(), + formatted_output: String::new(), + }), + source: ExecCommandSource::Agent, + start_time: None, + duration: None, + interaction_input: None, + tool_name: None, + tool_input: None, + tool_output: None, + tool_display_content: None, + }; + let cell = ExecCell::new(call, /*animations_enabled*/ false); + let rendered = cell + .command_display_lines(/*width*/ 80) + .iter() + .map(render_line_text) + .collect::>(); + + assert_eq!(rendered, vec!["▌ Ran echo hi"]); + } + // #[test] // fn truncate_lines_middle_keeps_omitted_count_in_line_units() { // let lines = vec![ From 70d9c3a657f28955c317167c5afd6ffca5495835 Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Thu, 16 Jul 2026 01:01:05 +0800 Subject: [PATCH 4/5] fix: synchronize managed model catalogs --- crates/cli/src/agent_command.rs | 2 +- crates/config/README.md | 7 + crates/core/Cargo.toml | 2 +- crates/core/src/model_catalog.rs | 173 +++++- crates/core/src/model_catalog/user_sync.rs | 638 +++++++++++++++++++++ docs/configuration.md | 17 + 6 files changed, 809 insertions(+), 30 deletions(-) create mode 100644 crates/core/src/model_catalog/user_sync.rs diff --git a/crates/cli/src/agent_command.rs b/crates/cli/src/agent_command.rs index f9c05dff..108254f9 100644 --- a/crates/cli/src/agent_command.rs +++ b/crates/cli/src/agent_command.rs @@ -39,7 +39,7 @@ pub(crate) async fn run_agent( .iter() .map(|warning| { format!( - "Skipped model catalog override {}: {}", + "Model catalog warning for {}: {}", warning.path.display(), warning.message ) diff --git a/crates/config/README.md b/crates/config/README.md index 208d59e2..95e5ffd6 100644 --- a/crates/config/README.md +++ b/crates/config/README.md @@ -462,6 +462,13 @@ defaults, merged by `slug`. Turn metadata records `model` as the catalog slug and `request_model` as the provider request model; these values may be identical. +Untouched built-in entries in `/models.json` are synchronized with +the catalog bundled in the running binary. Devo records the source fingerprint +and update policy in a reserved `_devo` object. Editing model fields preserves +the full entry as a user override. Set `_devo.update_policy` to `"pinned"` to +prevent an unchanged built-in entry from being refreshed explicitly. Custom +slugs and workspace-level catalogs are never rewritten by this synchronization. + When reasoning effort resolution selects a model variant catalog slug, the provider request model is resolved from enabled bindings for the same provider as the selected turn binding. Duplicate `model_slug` values under other providers do diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 62200e7d..5c74981c 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -42,6 +42,7 @@ serde_json = { workspace = true } sha2 = { workspace = true } shlex = { workspace = true } smol_str = { workspace = true } +tempfile = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true } tokio-stream = { workspace = true } @@ -60,6 +61,5 @@ devo-safety = { workspace = true } futures = { workspace = true } pretty_assertions = { workspace = true } serde_json = { workspace = true } -tempfile = { workspace = true } tokio = { workspace = true, features = ["full"] } uuid = { workspace = true } diff --git a/crates/core/src/model_catalog.rs b/crates/core/src/model_catalog.rs index eed2b312..6605deca 100644 --- a/crates/core/src/model_catalog.rs +++ b/crates/core/src/model_catalog.rs @@ -20,6 +20,9 @@ use std::path::{Path, PathBuf}; use crate::{Model, ModelCatalog, ModelError, ModelPreset}; +use serde_json::Value; + +mod user_sync; const BUILTIN_MODELS_JSON: &str = include_str!("../models.json"); @@ -59,25 +62,41 @@ impl PresetModelCatalog { /// Implementation loads from fallback to highest precedence so later /// layers can replace entries with the same slug. /// - /// If the user file does not exist it is seeded from the builtin list so - /// users can discover and customize the catalog. + /// The user file is synchronized with the built-in list while preserving + /// pinned, edited, and user-defined entries. pub fn load_from_config( config_home: &Path, workspace_root: Option<&Path>, ) -> Result { - seed_user_models_file(config_home); - let mut presets = load_builtin_model_presets()?; - let mut warnings = Vec::new(); - merge_filesystem_model_presets( - &mut presets, - &mut warnings, - &config_home.join("models.json"), - ); + let user_path = config_home.join("models.json"); + let project_path = + workspace_root.map(|workspace_root| workspace_root.join(".devo").join("models.json")); + let user_path_is_workspace_owned = project_path + .as_ref() + .is_some_and(|project_path| catalog_paths_alias(&user_path, project_path)); + + if !user_path_is_workspace_owned { + let builtin_entries: Vec = serde_json::from_str(BUILTIN_MODELS_JSON)?; + let user_sync = user_sync::synchronize_user_catalog(&user_path, &builtin_entries); + if let Some(user_presets) = user_sync.presets { + presets = merge_model_presets(presets, user_presets); + } + for message in user_sync.warnings { + tracing::warn!( + path = %user_path.display(), + warning = %message, + "model catalog synchronization warning" + ); + warnings.push(ModelCatalogWarning { + path: user_path.clone(), + message, + }); + } + } - if let Some(workspace_root) = workspace_root { - let project_path = workspace_root.join(".devo").join("models.json"); + if let Some(project_path) = project_path { merge_filesystem_model_presets(&mut presets, &mut warnings, &project_path); } @@ -159,6 +178,33 @@ fn load_models_from_file(path: &Path) -> Result>, ModelC .map_err(ModelCatalogFileError::Parse) } +fn catalog_paths_alias(left: &Path, right: &Path) -> bool { + if left == right { + return true; + } + if let (Ok(left), Ok(right)) = (std::fs::canonicalize(left), std::fs::canonicalize(right)) + && left == right + { + return true; + } + if left.file_name() != right.file_name() { + return false; + } + + match (left.parent(), right.parent()) { + (Some(left_parent), Some(right_parent)) => { + match ( + std::fs::canonicalize(left_parent), + std::fs::canonicalize(right_parent), + ) { + (Ok(left_parent), Ok(right_parent)) => left_parent == right_parent, + _ => false, + } + } + _ => false, + } +} + fn merge_filesystem_model_presets( presets: &mut Vec, warnings: &mut Vec, @@ -196,19 +242,6 @@ fn merge_model_presets(mut base: Vec, overlay: Vec) -> base } -/// Copies the built-in `models.json` to the user config directory if no user -/// file exists yet. -fn seed_user_models_file(config_home: &Path) { - let user_path = config_home.join("models.json"); - if user_path.exists() { - return; - } - if let Some(parent) = user_path.parent() { - let _ = std::fs::create_dir_all(parent); - } - let _ = std::fs::write(&user_path, BUILTIN_MODELS_JSON); -} - /// Errors produced while loading the builtin catalog. #[derive(Debug, thiserror::Error)] pub enum PresetModelCatalogError { @@ -327,7 +360,7 @@ mod tests { } #[test] - fn load_from_config_seeds_user_file_when_missing() { + fn load_from_config_creates_managed_user_file_when_missing() { let root = unique_temp_dir("catalog-seed"); let home = root.join("home").join(".devo"); std::fs::create_dir_all(&home).expect("create home"); @@ -340,13 +373,21 @@ mod tests { assert!(user_file.exists()); let contents = std::fs::read_to_string(&user_file).expect("read"); - assert!(contents.contains("qwen3-coder-next")); + let entries: Vec = + serde_json::from_str(&contents).expect("parse managed user catalog"); + assert!(!entries.is_empty()); + assert!(entries.iter().all(|entry| { + entry["_devo"]["update_policy"] == "managed" + && entry["_devo"]["builtin_sha256"] + .as_str() + .is_some_and(|hash| hash.starts_with("sha256:")) + })); let _ = std::fs::remove_dir_all(root); } #[test] - fn load_from_config_does_not_overwrite_existing_user_file() { + fn load_from_config_preserves_custom_models_when_appending_builtins() { let root = unique_temp_dir("catalog-no-overwrite"); let home = root.join("home").join(".devo"); std::fs::create_dir_all(&home).expect("create home"); @@ -364,6 +405,14 @@ mod tests { assert!(models.iter().any(|m| m.slug == "custom")); assert!(models.iter().any(|m| m.slug == "qwen3-coder-next")); + let entries: Vec = serde_json::from_str( + &std::fs::read_to_string(&user_file).expect("read synchronized user catalog"), + ) + .expect("parse synchronized user catalog"); + assert_eq!( + entries[0], + serde_json::json!({"slug": "custom", "display_name": "Custom"}) + ); let _ = std::fs::remove_dir_all(root); } @@ -456,6 +505,68 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + #[test] + fn load_from_config_does_not_sync_catalog_shared_by_user_and_workspace_paths() { + let root = unique_temp_dir("catalog-shared-user-workspace"); + let workspace = root.join("workspace"); + let shared_home = workspace.join(".devo"); + std::fs::create_dir_all(&shared_home).expect("create shared catalog directory"); + let shared_catalog = shared_home.join("models.json"); + let contents = r#"[{"slug":"qwen3-coder-next","display_name":"Workspace-owned catalog"}]"#; + std::fs::write(&shared_catalog, contents).expect("write shared catalog"); + + let catalog = + PresetModelCatalog::load_from_config(&shared_home, Some(&workspace)).expect("load"); + + assert_eq!( + catalog + .get("qwen3-coder-next") + .expect("workspace model") + .display_name, + "Workspace-owned catalog" + ); + assert_eq!( + std::fs::read_to_string(&shared_catalog).expect("read shared catalog"), + contents + ); + + let _ = std::fs::remove_dir_all(root); + } + + #[cfg(unix)] + #[test] + fn load_from_config_does_not_sync_symlinked_workspace_catalog() { + use std::os::unix::fs::symlink; + + let root = unique_temp_dir("catalog-symlinked-user-workspace"); + let config_home = root.join("catalog"); + let workspace = root.join("workspace"); + std::fs::create_dir_all(&config_home).expect("create catalog directory"); + std::fs::create_dir_all(&workspace).expect("create workspace"); + symlink(&config_home, workspace.join(".devo")).expect("symlink workspace catalog"); + let shared_catalog = config_home.join("models.json"); + let contents = + r#"[{"slug":"qwen3-coder-next","display_name":"Symlinked workspace catalog"}]"#; + std::fs::write(&shared_catalog, contents).expect("write shared catalog"); + + let catalog = + PresetModelCatalog::load_from_config(&config_home, Some(&workspace)).expect("load"); + + assert_eq!( + catalog + .get("qwen3-coder-next") + .expect("workspace model") + .display_name, + "Symlinked workspace catalog" + ); + assert_eq!( + std::fs::read_to_string(&shared_catalog).expect("read shared catalog"), + contents + ); + + let _ = std::fs::remove_dir_all(root); + } + #[test] fn load_from_config_uses_workspace_user_builtin_precedence_by_slug() { let root = unique_temp_dir("catalog-precedence"); @@ -476,6 +587,8 @@ mod tests { r#"[{"slug":"qwen3-coder-next","display_name":"Workspace","context_window":333,"effective_context_window_percent":88,"max_tokens":444}]"#, ) .expect("write project models"); + let workspace_contents = + std::fs::read_to_string(&workspace_models).expect("read project models before load"); let catalog = PresetModelCatalog::load_from_config(&home, Some(&workspace)).expect("load"); let model = model_by_slug(&catalog.into_inner(), "qwen3-coder-next"); @@ -492,6 +605,10 @@ mod tests { ..ModelPreset::default() }) ); + assert_eq!( + std::fs::read_to_string(&workspace_models).expect("read project models after load"), + workspace_contents + ); let _ = std::fs::remove_dir_all(root); } diff --git a/crates/core/src/model_catalog/user_sync.rs b/crates/core/src/model_catalog/user_sync.rs new file mode 100644 index 00000000..17e42172 --- /dev/null +++ b/crates/core/src/model_catalog/user_sync.rs @@ -0,0 +1,638 @@ +use std::collections::{HashMap, HashSet}; +use std::fs; +use std::io::Write; +use std::path::Path; + +use crate::ModelPreset; +use serde_json::{Map, Value}; +use sha2::{Digest, Sha256}; + +const METADATA_KEY: &str = "_devo"; +const BUILTIN_SHA256_KEY: &str = "builtin_sha256"; +const UPDATE_POLICY_KEY: &str = "update_policy"; +const MANAGED_POLICY: &str = "managed"; +const PINNED_POLICY: &str = "pinned"; + +#[derive(Debug)] +pub(super) struct UserCatalogSyncOutcome { + pub(super) presets: Option>, + pub(super) warnings: Vec, +} + +pub(super) fn synchronize_user_catalog( + user_path: &Path, + builtin_entries: &[Value], +) -> UserCatalogSyncOutcome { + synchronize_user_catalog_with_writer(user_path, builtin_entries, write_atomic) +} + +fn synchronize_user_catalog_with_writer( + user_path: &Path, + builtin_entries: &[Value], + writer: F, +) -> UserCatalogSyncOutcome +where + F: FnOnce(&Path, &[u8]) -> std::io::Result<()>, +{ + let (mut user_entries, existed) = match read_user_entries(user_path) { + Ok(entries) => entries, + Err(message) => { + return UserCatalogSyncOutcome { + presets: None, + warnings: vec![message], + }; + } + }; + + if let Err(error) = parse_presets(&user_entries) { + return UserCatalogSyncOutcome { + presets: None, + warnings: vec![format!("failed to parse model catalog: {error}")], + }; + } + + let original_entries = user_entries.clone(); + synchronize_entries(&mut user_entries, builtin_entries); + + let presets = match parse_presets(&user_entries) { + Ok(presets) => presets, + Err(error) => { + return UserCatalogSyncOutcome { + presets: None, + warnings: vec![format!( + "failed to parse synchronized model catalog: {error}" + )], + }; + } + }; + + let mut warnings = Vec::new(); + if !existed || user_entries != original_entries { + match serde_json::to_string_pretty(&user_entries) { + Ok(mut serialized) => { + serialized.push('\n'); + if let Err(error) = writer(user_path, serialized.as_bytes()) { + warnings.push(format!( + "failed to persist synchronized model catalog: {error}" + )); + } + } + Err(error) => warnings.push(format!( + "failed to serialize synchronized model catalog: {error}" + )), + } + } + + UserCatalogSyncOutcome { + presets: Some(presets), + warnings, + } +} + +fn read_user_entries(path: &Path) -> Result<(Vec, bool), String> { + let contents = match fs::read_to_string(path) { + Ok(contents) => contents, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Ok((Vec::new(), false)); + } + Err(error) => return Err(format!("failed to read model catalog: {error}")), + }; + + if contents.trim().is_empty() { + return Ok((Vec::new(), true)); + } + + serde_json::from_str(&contents) + .map(|entries| (entries, true)) + .map_err(|error| format!("failed to parse model catalog: {error}")) +} + +fn parse_presets(entries: &[Value]) -> Result, serde_json::Error> { + serde_json::from_value(Value::Array(entries.to_vec())) +} + +fn synchronize_entries(user_entries: &mut Vec, builtin_entries: &[Value]) { + let builtins_by_slug: HashMap<&str, (&Value, String)> = builtin_entries + .iter() + .filter_map(|entry| { + let slug = entry.get("slug")?.as_str()?; + Some((slug, (entry, fingerprint(entry)))) + }) + .collect(); + let mut existing_slugs = HashSet::new(); + + for user_entry in user_entries.iter_mut() { + let Some(slug) = user_entry + .get("slug") + .and_then(Value::as_str) + .map(str::to_owned) + else { + continue; + }; + existing_slugs.insert(slug.clone()); + + let Some((builtin_entry, builtin_hash)) = builtins_by_slug.get(slug.as_str()) else { + continue; + }; + + match management_state(user_entry) { + EntryManagement::Pinned => {} + EntryManagement::Managed { source_hash } => { + let user_hash = fingerprint(user_entry); + if user_hash == source_hash { + *user_entry = + managed_builtin_entry(builtin_entry, Some(user_entry), builtin_hash); + } + } + EntryManagement::Legacy => { + if fingerprint(user_entry) == *builtin_hash { + *user_entry = + managed_builtin_entry(builtin_entry, Some(user_entry), builtin_hash); + } else { + pin_entry(user_entry); + } + } + } + } + + for builtin_entry in builtin_entries { + let Some(slug) = builtin_entry.get("slug").and_then(Value::as_str) else { + continue; + }; + if existing_slugs.insert(slug.to_owned()) { + let builtin_hash = fingerprint(builtin_entry); + user_entries.push(managed_builtin_entry( + builtin_entry, + /*previous_entry*/ None, + &builtin_hash, + )); + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum EntryManagement { + Legacy, + Managed { source_hash: String }, + Pinned, +} + +fn management_state(entry: &Value) -> EntryManagement { + let Some(metadata) = entry.get(METADATA_KEY).and_then(Value::as_object) else { + return EntryManagement::Legacy; + }; + let policy = metadata.get(UPDATE_POLICY_KEY).and_then(Value::as_str); + let source_hash = metadata + .get(BUILTIN_SHA256_KEY) + .and_then(Value::as_str) + .filter(|hash| is_valid_builtin_hash(hash)) + .map(str::to_owned); + + match (policy, source_hash) { + (Some(PINNED_POLICY), _) => EntryManagement::Pinned, + (Some(MANAGED_POLICY), Some(source_hash)) => EntryManagement::Managed { source_hash }, + (Some(MANAGED_POLICY), None) => EntryManagement::Legacy, + (Some(_), _) => EntryManagement::Pinned, + (None, Some(source_hash)) => EntryManagement::Managed { source_hash }, + (None, None) => EntryManagement::Legacy, + } +} + +fn is_valid_builtin_hash(hash: &str) -> bool { + hash.strip_prefix("sha256:").is_some_and(|digest| { + digest.len() == 64 && digest.bytes().all(|byte| byte.is_ascii_hexdigit()) + }) +} + +fn fingerprint(entry: &Value) -> String { + let mut content = entry.clone(); + if let Some(object) = content.as_object_mut() { + object.remove(METADATA_KEY); + } + let canonical = canonicalize_json(&content); + let digest = Sha256::digest( + serde_json::to_vec(&canonical).expect("serializing serde_json::Value cannot fail"), + ); + format!("sha256:{digest:x}") +} + +fn canonicalize_json(value: &Value) -> Value { + match value { + Value::Array(values) => Value::Array(values.iter().map(canonicalize_json).collect()), + Value::Object(object) => { + let mut entries: Vec<_> = object.iter().collect(); + entries.sort_unstable_by(|(left, _), (right, _)| left.cmp(right)); + Value::Object( + entries + .into_iter() + .map(|(key, value)| (key.clone(), canonicalize_json(value))) + .collect(), + ) + } + Value::Number(number) => Value::Number(canonical_number(number)), + scalar => scalar.clone(), + } +} + +fn canonical_number(number: &serde_json::Number) -> serde_json::Number { + if let Some(value) = number.as_i64() { + return value.into(); + } + if let Some(value) = number.as_u64() { + return value.into(); + } + + let value = number + .as_f64() + .expect("serde_json numbers are finite numeric values"); + if value == 0.0 { + return 0.into(); + } + if value.fract() == 0.0 && value >= i64::MIN as f64 && value <= i64::MAX as f64 { + return (value as i64).into(); + } + serde_json::Number::from_f64(value).expect("serde_json numbers are finite") +} + +fn managed_builtin_entry( + builtin_entry: &Value, + previous_entry: Option<&Value>, + builtin_hash: &str, +) -> Value { + let mut managed = builtin_entry.clone(); + let object = managed + .as_object_mut() + .expect("built-in model entries must be JSON objects"); + object.remove(METADATA_KEY); + + let mut metadata = previous_entry + .and_then(|entry| entry.get(METADATA_KEY)) + .and_then(Value::as_object) + .cloned() + .unwrap_or_default(); + metadata.insert( + BUILTIN_SHA256_KEY.into(), + Value::String(builtin_hash.to_owned()), + ); + metadata.insert( + UPDATE_POLICY_KEY.into(), + Value::String(MANAGED_POLICY.into()), + ); + object.insert(METADATA_KEY.into(), Value::Object(metadata)); + managed +} + +fn pin_entry(entry: &mut Value) { + let object = entry + .as_object_mut() + .expect("validated model entries must be JSON objects"); + let metadata = object + .entry(METADATA_KEY) + .or_insert_with(|| Value::Object(Map::new())); + if !metadata.is_object() { + *metadata = Value::Object(Map::new()); + } + metadata.as_object_mut().expect("metadata object").insert( + UPDATE_POLICY_KEY.into(), + Value::String(PINNED_POLICY.into()), + ); +} + +fn write_atomic(path: &Path, data: &[u8]) -> std::io::Result<()> { + let parent = path.parent().unwrap_or_else(|| Path::new(".")); + fs::create_dir_all(parent)?; + let mut temp_file = tempfile::NamedTempFile::new_in(parent)?; + temp_file.write_all(data)?; + temp_file.as_file().sync_all()?; + temp_file + .persist(path) + .map(|_persisted_file| ()) + .map_err(|error| error.error) +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::Path; + + use crate::ModelPreset; + use pretty_assertions::assert_eq; + use serde_json::{Value, json}; + use sha2::{Digest, Sha256}; + use tempfile::TempDir; + + use super::{synchronize_user_catalog, synchronize_user_catalog_with_writer}; + + fn model(slug: &str, display_name: &str) -> Value { + json!({ + "slug": slug, + "display_name": display_name, + "provider": "openai_chat_completions", + "context_window": 200000, + "input_modalities": ["text"] + }) + } + + fn sha256(entry: &Value) -> String { + let mut canonical = entry.clone(); + canonical + .as_object_mut() + .expect("model entry object") + .remove("_devo"); + let digest = Sha256::digest(serde_json::to_vec(&canonical).expect("serialize model")); + format!("sha256:{digest:x}") + } + + fn managed(entry: &Value) -> Value { + let mut expected = entry.clone(); + expected + .as_object_mut() + .expect("model entry object") + .insert( + "_devo".into(), + json!({ + "builtin_sha256": sha256(entry), + "update_policy": "managed" + }), + ); + expected + } + + fn pinned(entry: &Value) -> Value { + let mut expected = entry.clone(); + expected + .as_object_mut() + .expect("model entry object") + .insert( + "_devo".into(), + json!({ + "update_policy": "pinned" + }), + ); + expected + } + + fn read_entries(path: &Path) -> Vec { + serde_json::from_str(&fs::read_to_string(path).expect("read catalog")) + .expect("parse catalog") + } + + fn write_entries(path: &Path, entries: &[Value]) { + fs::write( + path, + serde_json::to_string_pretty(entries).expect("serialize catalog"), + ) + .expect("write catalog"); + } + + #[test] + fn missing_user_catalog_creates_managed_builtin_copy() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("nested").join("models.json"); + let builtins = vec![model("a", "Builtin A"), model("b", "Builtin B")]; + + let outcome = synchronize_user_catalog(&path, &builtins); + + assert_eq!(outcome.warnings, Vec::::new()); + assert!(outcome.presets.is_some()); + assert_eq!( + read_entries(&path), + builtins.iter().map(managed).collect::>() + ); + } + + #[test] + fn untouched_managed_entry_tracks_changed_builtin() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let previous = model("a", "Previous"); + write_entries(&path, &[managed(&previous)]); + let current = model("a", "Current"); + + let outcome = synchronize_user_catalog(&path, std::slice::from_ref(¤t)); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![managed(¤t)]); + } + + #[test] + fn edited_managed_entry_is_preserved() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let previous = model("a", "Previous"); + let mut edited = managed(&previous); + edited["display_name"] = json!("User edit"); + write_entries(&path, std::slice::from_ref(&edited)); + let current = model("a", "Current"); + + let outcome = synchronize_user_catalog(&path, &[current]); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![edited]); + } + + #[test] + fn explicitly_pinned_entry_is_preserved() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let user = pinned(&model("a", "Pinned")); + write_entries(&path, std::slice::from_ref(&user)); + + let outcome = synchronize_user_catalog(&path, &[model("a", "Current")]); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![user]); + } + + #[test] + fn invalid_managed_fingerprints_are_migrated_conservatively() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let mut missing = model("a", "Missing hash user edit"); + missing["_devo"] = json!({"update_policy": "managed"}); + let mut non_string = model("a", "Non-string hash user edit"); + non_string["_devo"] = json!({ + "builtin_sha256": 42, + "update_policy": "managed" + }); + let mut malformed = model("a", "Malformed hash user edit"); + malformed["_devo"] = json!({ + "builtin_sha256": "sha256:not-a-valid-digest", + "update_policy": "managed" + }); + write_entries( + &path, + &[missing.clone(), non_string.clone(), malformed.clone()], + ); + missing["_devo"]["update_policy"] = json!("pinned"); + non_string["_devo"]["update_policy"] = json!("pinned"); + malformed["_devo"]["update_policy"] = json!("pinned"); + + let outcome = synchronize_user_catalog(&path, &[model("a", "Builtin")]); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![missing, non_string, malformed]); + } + + #[test] + fn legacy_entries_are_classified_without_losing_custom_models() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let unchanged = model("a", "Builtin A"); + let changed = model("b", "User B"); + let custom = model("custom", "Custom"); + write_entries(&path, &[unchanged.clone(), changed.clone(), custom.clone()]); + let builtins = vec![unchanged.clone(), model("b", "Builtin B")]; + + let outcome = synchronize_user_catalog(&path, &builtins); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!( + read_entries(&path), + vec![managed(&unchanged), pinned(&changed), custom] + ); + } + + #[test] + fn new_builtins_are_appended_and_removed_builtins_are_preserved() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let removed = managed(&model("removed", "Removed")); + write_entries(&path, std::slice::from_ref(&removed)); + let added = model("added", "Added"); + + let outcome = synchronize_user_catalog(&path, std::slice::from_ref(&added)); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![removed, managed(&added)]); + } + + #[test] + fn json_formatting_and_key_order_do_not_count_as_edits() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let mut previous = model("a", "Previous"); + previous["nested"] = json!({"alpha": 1, "beta": 2}); + let previous_hash = sha256(&previous); + fs::write( + &path, + format!( + r#"[{{"provider":"openai_chat_completions","slug":"a","nested":{{"beta":2,"alpha":1}},"input_modalities":["text"],"context_window":200000,"display_name":"Previous","_devo":{{"update_policy":"managed","builtin_sha256":"{previous_hash}"}}}}]"# + ), + ) + .expect("write reordered catalog"); + let current = model("a", "Current"); + + let outcome = synchronize_user_catalog(&path, std::slice::from_ref(¤t)); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![managed(¤t)]); + } + + #[test] + fn equivalent_number_representations_do_not_count_as_edits() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let mut previous = model("a", "Previous"); + previous["temperature"] = json!(1); + let mut user_entry = managed(&previous); + user_entry["temperature"] = json!(1.0); + write_entries(&path, &[user_entry]); + let mut current = previous; + current["display_name"] = json!("Current"); + + let outcome = synchronize_user_catalog(&path, std::slice::from_ref(¤t)); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![managed(¤t)]); + } + + #[test] + fn unchanged_managed_catalog_is_not_rewritten() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let current = model("a", "Current"); + let managed_current = managed(¤t); + let compact = + serde_json::to_string(&vec![managed_current]).expect("serialize compact JSON"); + fs::write(&path, &compact).expect("write compact catalog"); + + let outcome = synchronize_user_catalog(&path, std::slice::from_ref(¤t)); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(fs::read_to_string(&path).expect("read catalog"), compact); + } + + #[test] + fn managed_updates_preserve_unknown_metadata_fields() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let previous = model("a", "Previous"); + let mut managed_previous = managed(&previous); + managed_previous["_devo"]["future_metadata"] = json!({"keep": true}); + write_entries(&path, &[managed_previous]); + let current = model("a", "Current"); + let mut expected = managed(¤t); + expected["_devo"]["future_metadata"] = json!({"keep": true}); + + let outcome = synchronize_user_catalog(&path, std::slice::from_ref(¤t)); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![expected]); + } + + #[test] + fn unknown_model_fields_count_as_user_customization() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let previous = model("a", "Previous"); + let mut customized = managed(&previous); + customized["future_provider_option"] = json!({"enabled": true}); + write_entries(&path, std::slice::from_ref(&customized)); + + let outcome = synchronize_user_catalog(&path, &[model("a", "Current")]); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!(read_entries(&path), vec![customized]); + } + + #[test] + fn invalid_user_json_is_not_overwritten() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let invalid = "{not valid json"; + fs::write(&path, invalid).expect("write invalid catalog"); + + let outcome = synchronize_user_catalog(&path, &[model("a", "Builtin")]); + + assert_eq!(outcome.presets, None); + assert_eq!(outcome.warnings.len(), 1); + assert!(outcome.warnings[0].contains("failed to parse model catalog")); + assert_eq!( + fs::read_to_string(&path).expect("read invalid catalog"), + invalid + ); + } + + #[test] + fn write_failure_returns_synchronized_presets_and_warning() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("models.json"); + let previous = model("a", "Previous"); + write_entries(&path, &[managed(&previous)]); + let current = model("a", "Current"); + + let outcome = synchronize_user_catalog_with_writer( + &path, + std::slice::from_ref(¤t), + |_path, _data| Err(std::io::Error::other("injected write failure")), + ); + + assert_eq!(outcome.warnings.len(), 1); + assert!(outcome.warnings[0].contains("failed to persist synchronized model catalog")); + assert_eq!(read_entries(&path), vec![managed(&previous)]); + let expected_presets: Vec = + serde_json::from_value(Value::Array(vec![current])).expect("parse current preset"); + assert_eq!(outcome.presets, Some(expected_presets)); + } +} diff --git a/docs/configuration.md b/docs/configuration.md index 77fb1ded..9825226f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -63,6 +63,23 @@ User-level model catalog: Project-level overrides can also be placed at `/.devo/models.json`. Catalog precedence is `/.devo/models.json`, then `/models.json`, then the built-in catalog. + +Devo keeps untouched built-in entries in the user-level catalog synchronized +with the catalog bundled in the running binary. Managed entries contain a +reserved `_devo` object with a content fingerprint. Editing any model field +turns that entry into an effective user override, so later built-in changes do +not replace it. To pin an otherwise unchanged entry explicitly, set: + +```json +"_devo": { + "update_policy": "pinned" +} +``` + +Custom slugs are never managed, and project-level catalogs are never rewritten. +Removing the pin and deleting the customized entry lets Devo add the current +built-in entry again on the next startup. + In `models.json`, `provider` is the default wire API metadata for the model; the actual endpoint is still selected by the `provider` field in `config.toml`. If `base_instructions` is omitted, Devo falls back to the built-in default base From b8da44bdb68d9d1099441391a86a820cef7d57d4 Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Thu, 16 Jul 2026 01:15:23 +0800 Subject: [PATCH 5/5] test: cover update_goal loop termination --- crates/server/tests/goal_update_completion.rs | 198 ++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 crates/server/tests/goal_update_completion.rs diff --git a/crates/server/tests/goal_update_completion.rs b/crates/server/tests/goal_update_completion.rs new file mode 100644 index 00000000..885e2b45 --- /dev/null +++ b/crates/server/tests/goal_update_completion.rs @@ -0,0 +1,198 @@ +use std::pin::Pin; +use std::sync::Arc; +use std::sync::Mutex; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; +use std::time::Duration; + +use anyhow::Context; +use anyhow::Result; +use async_trait::async_trait; +use devo_core::tools::create_default_tool_registry; +use devo_protocol::ModelRequest; +use devo_protocol::ModelResponse; +use devo_protocol::ResponseContent; +use devo_protocol::ResponseMetadata; +use devo_protocol::StopReason; +use devo_protocol::StreamEvent; +use devo_protocol::Usage; +use devo_provider::ModelProviderSDK; +use futures::Stream; +use futures::stream; +use pretty_assertions::assert_eq; +use tempfile::TempDir; +use tokio::sync::Notify; +use tokio::time::timeout; + +#[path = "support/goal_continuation.rs"] +mod support; + +use support::build_runtime_with_registry; +use support::collect_until_turn_completed; +use support::initialize_connection; +use support::start_session; +use support::wait_for_captured_request_count; + +#[derive(Default)] +struct CompletingGoalProvider { + requests: AtomicUsize, + captured_requests: Mutex>, + unexpected_request: Notify, +} + +#[async_trait] +impl ModelProviderSDK for CompletingGoalProvider { + async fn completion(&self, _request: ModelRequest) -> Result { + Ok(text_response("goal-title", "Completed goal")) + } + + async fn completion_stream( + &self, + request: ModelRequest, + ) -> Result> + Send>>> { + let request_number = self.requests.fetch_add(1, Ordering::SeqCst) + 1; + self.captured_requests + .lock() + .expect("lock captured requests") + .push(request); + + match request_number { + 1 => { + let input = serde_json::json!({ "status": "complete" }); + Ok(Box::pin(stream::iter(vec![ + Ok(StreamEvent::ToolCallStart { + index: 0, + id: "complete-goal".to_string(), + name: "update_goal".to_string(), + input: input.clone(), + }), + Ok(StreamEvent::MessageDone { + response: ModelResponse { + id: "update-goal-response".to_string(), + content: vec![ResponseContent::ToolUse { + id: "complete-goal".to_string(), + name: "update_goal".to_string(), + input, + }], + stop_reason: Some(StopReason::ToolUse), + usage: Usage::default(), + metadata: ResponseMetadata::default(), + }, + }), + ]))) + } + 2 => Ok(Box::pin(stream::iter(vec![ + Ok(StreamEvent::TextDelta { + index: 0, + text: "The goal is complete.".to_string(), + }), + Ok(StreamEvent::MessageDone { + response: text_response("final-response", "The goal is complete."), + }), + ]))), + _ => { + self.unexpected_request.notify_one(); + Ok(Box::pin(stream::pending())) + } + } + } + + fn name(&self) -> &str { + "completing-goal-provider" + } +} + +#[tokio::test] +async fn update_goal_completion_finishes_current_turn_without_another_continuation() -> Result<()> { + let data_root = TempDir::new()?; + let provider = Arc::new(CompletingGoalProvider::default()); + let runtime = build_runtime_with_registry( + data_root.path(), + provider.clone(), + Arc::new(create_default_tool_registry()), + )?; + let (connection_id, mut notifications_rx) = initialize_connection(&runtime).await?; + let session_id = start_session(&runtime, connection_id, data_root.path()).await?; + + runtime + .handle_incoming( + connection_id, + serde_json::json!({ + "jsonrpc": "2.0", + "id": 20, + "method": "_devo/goal/set", + "params": { + "sessionId": session_id, + "objective": "complete the goal with update_goal", + "status": "active" + } + }), + ) + .await + .context("goal/set response")?; + + collect_until_turn_completed(&mut notifications_rx).await?; + wait_for_captured_request_count(&provider.captured_requests, /*expected*/ 2).await?; + + let status_response = runtime + .handle_incoming( + connection_id, + serde_json::json!({ + "jsonrpc": "2.0", + "id": 21, + "method": "_devo/goal/status", + "params": { + "sessionId": session_id + } + }), + ) + .await + .context("goal/status response")?; + let response: devo_server::SuccessResponse = + serde_json::from_value(status_response)?; + assert_eq!( + response.result.goal.map(|goal| goal.status), + Some(devo_protocol::ThreadGoalStatus::Complete) + ); + + { + let requests = provider + .captured_requests + .lock() + .expect("lock captured requests"); + assert!( + requests[1].messages.iter().any(|message| { + message.content.iter().any(|content| { + matches!( + content, + devo_protocol::RequestContent::ToolResult { content, .. } + if content.contains("Goal marked complete") + ) + }) + }), + "the follow-up request should contain the successful update_goal result" + ); + } + + assert!( + timeout( + Duration::from_secs(/*secs*/ 1), + provider.unexpected_request.notified() + ) + .await + .is_err(), + "a completed goal must not start another provider request" + ); + assert_eq!(provider.requests.load(Ordering::SeqCst), 2); + Ok(()) +} + +fn text_response(id: &str, text: &str) -> ModelResponse { + ModelResponse { + id: id.to_string(), + content: vec![ResponseContent::Text(text.to_string())], + stop_reason: Some(StopReason::EndTurn), + usage: Usage::default(), + metadata: ResponseMetadata::default(), + } +}