From 57cd0ee46e70fd0c9b55bc090a1c33ee11c5dff5 Mon Sep 17 00:00:00 2001 From: Adrian Demian <8708742+adriandemian@users.noreply.github.com> Date: Fri, 19 Jun 2026 10:12:02 +0300 Subject: [PATCH] Fix ideation backend reliability regressions --- .../src/__tests__/tools.test.ts | 12 +++-- .../app/ralphx-mcp-server/src/plan-tools.ts | 17 ++++-- .../chat_service/chat_service_handlers.rs | 34 ++++++++++++ .../chat_service_handlers_tests.rs | 24 +++++++++ .../chat_service_send_background.rs | 10 ++++ .../git_service/tests/worktree_tests.rs | 52 +++++++++++++++++++ .../src/application/git_service/worktree.rs | 25 +++++++++ 7 files changed, 167 insertions(+), 7 deletions(-) diff --git a/plugins/app/ralphx-mcp-server/src/__tests__/tools.test.ts b/plugins/app/ralphx-mcp-server/src/__tests__/tools.test.ts index 4728323ba7..e5caa206b3 100644 --- a/plugins/app/ralphx-mcp-server/src/__tests__/tools.test.ts +++ b/plugins/app/ralphx-mcp-server/src/__tests__/tools.test.ts @@ -805,11 +805,15 @@ describe('New team tool definitions', () => { describe('get_plan_verification', () => { const tool = PLAN_TOOLS.find((t) => t.name === 'get_plan_verification'); - it('should derive the parent session automatically for verifier-owned reads', () => { + it('should expose session_id for parent ideation reads while allowing verifier-owned derivation', () => { expect(tool).toBeDefined(); - expect(tool?.description).toContain('do not pass session_id'); - expect((tool?.inputSchema.properties as any)).toEqual({}); - expect((tool?.inputSchema as any).examples?.[0]).toEqual({}); + expect(tool?.description).toContain('Parent ideation agents must pass session_id'); + expect(tool?.description).toContain('Verifier child agents omit session_id'); + expect((tool?.inputSchema.properties as any)).toHaveProperty('session_id'); + expect((tool?.inputSchema as any).examples?.[0]).toMatchObject({ + session_id: 'ideation-session-id', + }); + expect((tool?.inputSchema as any).examples?.[1]).toEqual({}); expect(tool?.inputSchema.required).toEqual([]); }); }); diff --git a/plugins/app/ralphx-mcp-server/src/plan-tools.ts b/plugins/app/ralphx-mcp-server/src/plan-tools.ts index dbcc9a5587..c349d95ce5 100644 --- a/plugins/app/ralphx-mcp-server/src/plan-tools.ts +++ b/plugins/app/ralphx-mcp-server/src/plan-tools.ts @@ -299,12 +299,23 @@ export const PLAN_TOOLS: Tool[] = [ { name: "get_plan_verification", description: - "Get the current verification status for the PARENT ideation session. Use this before and during verification to confirm the generation, in_progress flag, and current round before calling report_verification_round or complete_plan_verification. The canonical parent session is derived automatically from the active verification child context, so do not pass session_id. " + + "Get the current verification status for an ideation plan. Parent ideation agents must pass session_id. Verifier child agents omit session_id because the canonical parent session is derived automatically from the active verification child context. Use this before and during verification to confirm the generation, in_progress flag, and current round before calling report_verification_round or complete_plan_verification. " + "If a verification update call is rejected, call this again on the parent session and copy the returned generation/in_progress values instead of guessing.", inputSchema: { type: "object", - examples: [{}], - properties: {}, + examples: [ + { + session_id: "ideation-session-id", + }, + {}, + ], + properties: { + session_id: { + type: "string", + description: + "Parent ideation session ID. Required for parent ideation agents; verifier child agents omit it.", + }, + }, required: [], }, }, diff --git a/src-tauri/src/application/chat_service/chat_service_handlers.rs b/src-tauri/src/application/chat_service/chat_service_handlers.rs index 2cc6dedc4f..23e863c286 100644 --- a/src-tauri/src/application/chat_service/chat_service_handlers.rs +++ b/src-tauri/src/application/chat_service/chat_service_handlers.rs @@ -501,6 +501,16 @@ fn incomplete_review_action( } } +fn should_suppress_post_success_review_error_note( + context_type: ChatContextType, + existing_content: &str, + task_status: Option, +) -> bool { + context_type == ChatContextType::Review + && !existing_content.trim().is_empty() + && task_status.is_some_and(|status| status != InternalStatus::Reviewing) +} + pub(super) async fn apply_system_wide_provider_pause( app_handle: &Option>, category: &super::ProviderErrorCategory, @@ -1974,6 +1984,22 @@ pub(super) async fn handle_stream_error( stream_error, Some(StreamError::AgentExit { stderr, .. }) if is_nonfatal_mcp_tool_cancellation(stderr) ); + let review_task_status = if context_type == ChatContextType::Review { + match task_repo + .get_by_id(&TaskId::from_string(context_id.to_string())) + .await + { + Ok(Some(task)) => Some(task.internal_status), + _ => None, + } + } else { + None + }; + let suppress_post_success_review_error_note = should_suppress_post_success_review_error_note( + context_type, + &existing_content, + review_task_status, + ); let error_note = if suppress_transcript_error_note { tracing::info!( conversation_id = conversation_id.as_str(), @@ -1982,6 +2008,14 @@ pub(super) async fn handle_stream_error( "Suppressing non-fatal MCP cancellation note from persisted assistant transcript" ); existing_content + } else if suppress_post_success_review_error_note { + tracing::info!( + conversation_id = conversation_id.as_str(), + context_type = %context_type, + context_id, + "Suppressing terminal review stderr note because review content exists and task already left Reviewing" + ); + existing_content } else if existing_content.is_empty() { format!("{} {}]", super::AGENT_ERROR_PREFIX, redacted_error) } else { diff --git a/src-tauri/src/application/chat_service/chat_service_handlers_tests.rs b/src-tauri/src/application/chat_service/chat_service_handlers_tests.rs index 680904819e..b0b426bf7b 100644 --- a/src-tauri/src/application/chat_service/chat_service_handlers_tests.rs +++ b/src-tauri/src/application/chat_service/chat_service_handlers_tests.rs @@ -493,6 +493,30 @@ fn test_incomplete_review_action_escalates_only_for_live_reviewing_tasks() { ); } +#[test] +fn test_post_success_review_error_note_suppression_only_after_reviewing() { + assert!(should_suppress_post_success_review_error_note( + ChatContextType::Review, + "Review approved. No blocking issues.", + Some(InternalStatus::PendingMerge), + )); + assert!(!should_suppress_post_success_review_error_note( + ChatContextType::Review, + "Review approved. No blocking issues.", + Some(InternalStatus::Reviewing), + )); + assert!(!should_suppress_post_success_review_error_note( + ChatContextType::Review, + "", + Some(InternalStatus::PendingMerge), + )); + assert!(!should_suppress_post_success_review_error_note( + ChatContextType::TaskExecution, + "Useful output", + Some(InternalStatus::PendingMerge), + )); +} + #[tokio::test] async fn test_apply_system_wide_provider_pause_pauses_mixed_active_task_states() { let app_state = AppState::new_test(); diff --git a/src-tauri/src/application/chat_service/chat_service_send_background.rs b/src-tauri/src/application/chat_service/chat_service_send_background.rs index e1477e1f62..0fb2e80c10 100644 --- a/src-tauri/src/application/chat_service/chat_service_send_background.rs +++ b/src-tauri/src/application/chat_service/chat_service_send_background.rs @@ -1235,6 +1235,16 @@ pub fn spawn_send_message_background(ctx: BackgroundRunContext) { "Skipping post-loop finalization — {} turn(s) already finalized in stream loop", turns_finalized, ); + finalize_no_output_assistant_message( + &chat_message_repo, + &chat_timeline_repo, + app_handle.as_ref(), + &event_ctx, + &conversation_id, + &pre_assistant_msg_id, + &assistant_role, + ) + .await; } else if has_output { finalize_structured_assistant_message( &chat_message_repo, diff --git a/src-tauri/src/application/git_service/tests/worktree_tests.rs b/src-tauri/src/application/git_service/tests/worktree_tests.rs index 2181fff907..fbcfc3735e 100644 --- a/src-tauri/src/application/git_service/tests/worktree_tests.rs +++ b/src-tauri/src/application/git_service/tests/worktree_tests.rs @@ -949,6 +949,58 @@ async fn test_checkout_existing_branch_worktree_retries_after_locked_stale_entry let _ = GitService::delete_worktree(repo, &wt_path).await; } +/// checkout_existing_branch_worktree: target path is missing but still registered → prune + retry. +#[tokio::test] +async fn test_checkout_existing_branch_worktree_retries_after_missing_registered_entry() { + let temp_dir = tempfile::tempdir().unwrap(); + let repo = temp_dir.path(); + init_git_repo(repo); + + let wt_path = temp_dir.path().join("worktrees").join("missing-registered-wt"); + + Command::new("git") + .args(["branch", "temp-branch"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["branch", "target-branch"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args([ + "worktree", + "add", + wt_path.to_str().unwrap(), + "temp-branch", + ]) + .current_dir(repo) + .output() + .unwrap(); + assert!(wt_path.exists(), "Worktree should exist before deletion"); + + std::fs::remove_dir_all(&wt_path).unwrap(); + assert!( + !wt_path.exists(), + "Directory should be gone while metadata remains" + ); + + let result = + GitService::checkout_existing_branch_worktree(repo, &wt_path, "target-branch").await; + assert!( + result.is_ok(), + "checkout_existing_branch_worktree should prune missing registered metadata and retry: {:?}", + result.err() + ); + + assert!(wt_path.exists(), "Worktree should exist after retry"); + let branch = GitService::get_current_branch(&wt_path).await.unwrap(); + assert_eq!(branch, "target-branch", "Worktree should be on target branch"); + + let _ = GitService::delete_worktree(repo, &wt_path).await; +} + /// checkout_existing_branch_worktree: branch already checked out at another path → recovery. /// /// Scenario: a stale worktree from a prior execution still has the task branch checked out. diff --git a/src-tauri/src/application/git_service/worktree.rs b/src-tauri/src/application/git_service/worktree.rs index ae4ed2a37b..9842115254 100644 --- a/src-tauri/src/application/git_service/worktree.rs +++ b/src-tauri/src/application/git_service/worktree.rs @@ -1,6 +1,10 @@ use super::git_cmd; use super::*; +fn is_missing_registered_worktree_error(stderr: &str) -> bool { + stderr.contains("missing but already registered worktree") +} + impl GitService { // ========================================================================= // Worktree Operations (Worktree mode only) @@ -240,6 +244,27 @@ impl GitService { return Ok(()); } + // Guard: git metadata still has a registered worktree entry whose + // directory has been deleted. Git suggests prune/remove; prune is + // sufficient for the stale-missing case and keeps the target branch. + if is_missing_registered_worktree_error(&stderr) { + debug!( + "checkout_existing_branch_worktree: missing registered worktree at {:?}, pruning stale metadata and retrying", + worktree + ); + let _ = git_cmd::run(&["worktree", "prune"], repo).await; + + let retry = git_cmd::run(&args, repo).await?; + if !retry.status.success() { + let retry_stderr = String::from_utf8_lossy(&retry.stderr); + return Err(AppError::GitOperation(format!( + "Failed to create worktree at {:?} for branch '{}' after missing-registered prune retry: {}", + worktree, branch, retry_stderr + ))); + } + return Ok(()); + } + // Guard: branch is already checked out in another worktree (stale/orphan from prior run). // Extract the other worktree path from the error, delete it, prune, and retry. //