fix: make background result retriggers deterministic#231
Conversation
Queue completed branch and worker outputs as structured retrigger payloads and preserve real user prompts on PromptCancelled turns so channels relay fresh results without losing context.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
WalkthroughThe changes introduce a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| let status = if r.success { "completed" } else { "failed" }; | ||
| let summary = format!( | ||
| "[Background {} {} {}]: {}", | ||
| r.process_type, r.process_id, status, r.result | ||
| ); |
There was a problem hiding this comment.
If retrigger cap is hit, this path injects raw results into history. Might be worth truncating here (similar to retrigger_result_summary) to avoid blowing up context when a worker returns something huge.
| let status = if r.success { "completed" } else { "failed" }; | |
| let summary = format!( | |
| "[Background {} {} {}]: {}", | |
| r.process_type, r.process_id, status, r.result | |
| ); | |
| let status = if r.success { "completed" } else { "failed" }; | |
| let truncated = if r.result.len() > 500 { | |
| let boundary = r.result.floor_char_boundary(500); | |
| format!("{}... [truncated]", &r.result[..boundary]) | |
| } else { | |
| r.result.clone() | |
| }; | |
| let summary = format!( | |
| "[Background {} {} {}]: {}", | |
| r.process_type, r.process_id, status, truncated | |
| ); |
| let result_count = self.pending_results.len(); | ||
|
|
||
| // Build per-result summaries for the template. | ||
| let result_items: Vec<_> = self | ||
| .pending_results | ||
| .iter() | ||
| .map(|r| crate::prompts::engine::RetriggerResult { | ||
| process_type: r.process_type.to_string(), | ||
| process_id: r.process_id.clone(), | ||
| success: r.success, | ||
| result: r.result.clone(), | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
Since the PR goal is deterministic handling, consider sorting the pending results before templating so equivalent result sets render in a stable order (even if completion events arrive in a different sequence).
| let result_count = self.pending_results.len(); | |
| // Build per-result summaries for the template. | |
| let result_items: Vec<_> = self | |
| .pending_results | |
| .iter() | |
| .map(|r| crate::prompts::engine::RetriggerResult { | |
| process_type: r.process_type.to_string(), | |
| process_id: r.process_id.clone(), | |
| success: r.success, | |
| result: r.result.clone(), | |
| }) | |
| .collect(); | |
| let result_count = self.pending_results.len(); | |
| // Keep ordering deterministic so equivalent retrigger batches render consistently. | |
| self.pending_results.sort_by(|a, b| { | |
| (a.process_type, a.process_id.as_str()).cmp(&(b.process_type, b.process_id.as_str())) | |
| }); | |
| // Build per-result summaries for the template. | |
| let result_items: Vec<_> = self | |
| .pending_results | |
| .iter() | |
| .map(|r| crate::prompts::engine::RetriggerResult { | |
| process_type: r.process_type.to_string(), | |
| process_id: r.process_id.clone(), | |
| success: r.success, | |
| result: r.result.clone(), | |
| }) | |
| .collect(); |
…ger-fix fix: make background result retriggers deterministic
Summary
ChannelDetailUI handling to reflect the retrigger flow changes exposed by the backend.Note
Implementation Details
This PR refactors background process result handling to eliminate non-determinism in retrigger behavior. Instead of directly injecting results into conversation history as fake user messages (which could be confused with prior results), the channel now accumulates results in a
PendingResultqueue and embeds them directly into the retrigger message. This ensures the LLM sees exactly which process completed and what it returned, with unambiguous process type and ID tags.Key changes: (1) Introduced
PendingResultstruct to queue branch/worker results with metadata, (2) Modifiedflush_pending_retrigger()to render results into the retrigger template for consistent message formatting, (3) Updatedapply_history_after_turn()to handle retrigger cancellations separately and preserve user prompts while discarding dangling assistant tool-calls, (4) Enhanced prompt templates to embed structured result data instead of plain text, and (5) Updated UI to make worker result display collapsible with an "Open" button instead of embedding task details inline.Written by Tembo for commit 141fce5. This will update automatically on new commits.