Skip to content

Commit 5f5a0b0

Browse files
PureWeenCopilot
andcommitted
Fix review findings: track successful workers, don't skip cleanup
- Track only successfully completed workers (not just dispatched) so failed workers don't count as having participated - Replace continue statements with flow-through logic so stall detection, SaveOrganization, and UI updates always run - Add comment documenting HashSet sequential access safety Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 21e47ac commit 5f5a0b0

1 file changed

Lines changed: 22 additions & 15 deletions

File tree

PolyPilot/Services/CopilotService.Organization.cs

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1681,6 +1681,8 @@ private async Task SendViaOrchestratorReflectAsync(string groupId, List<string>
16811681
}
16821682

16831683
var workerNames = members.Where(m => m != orchestratorName).ToList();
1684+
// Tracks workers that have successfully completed at least once across all iterations.
1685+
// Access is sequential (single async flow, no concurrent modification).
16841686
var dispatchedWorkers = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
16851687

16861688
try
@@ -1761,9 +1763,9 @@ private async Task SendViaOrchestratorReflectAsync(string groupId, List<string>
17611763
var workerTasks = assignments.Select(a => ExecuteWorkerAsync(a.WorkerName, a.Task, prompt, ct));
17621764
var results = await Task.WhenAll(workerTasks);
17631765

1764-
// Track which workers have been dispatched across all iterations
1765-
foreach (var a in assignments)
1766-
dispatchedWorkers.Add(a.WorkerName);
1766+
// Track only workers that succeeded across all iterations
1767+
foreach (var r in results.Where(r => r.Success))
1768+
dispatchedWorkers.Add(r.WorkerName);
17671769

17681770
// Phase 4: Synthesize + Evaluate
17691771
InvokeOnUI(() => OnOrchestratorPhaseChanged?.Invoke(groupId, OrchestratorPhase.Synthesizing, iterDetail));
@@ -1799,18 +1801,21 @@ private async Task SendViaOrchestratorReflectAsync(string groupId, List<string>
17991801
AddOrchestratorSystemMessage(orchestratorName, $"✅ {reflectState.BuildCompletionSummary()} (score: {score:F1})");
18001802
break;
18011803
}
1802-
else if (!allWorkersDispatched)
1804+
1805+
if (!allWorkersDispatched)
18031806
{
18041807
var missing = workerNames.Where(w => !dispatchedWorkers.Contains(w)).ToList();
18051808
Debug($"Reflection: overriding completion — workers not yet dispatched: {string.Join(", ", missing)}");
18061809
reflectState.LastEvaluation = $"Not all workers have participated yet. Missing: {string.Join(", ", missing)}. " +
18071810
"Dispatch to the remaining workers before completing.";
18081811
AddOrchestratorSystemMessage(orchestratorName,
18091812
$"🔄 Overriding completion — {string.Join(", ", missing)} haven't participated yet.");
1810-
continue;
1813+
}
1814+
else
1815+
{
1816+
reflectState.LastEvaluation = rationale;
18111817
}
18121818

1813-
reflectState.LastEvaluation = rationale;
18141819
if (trend == Models.QualityTrend.Degrading)
18151820
reflectState.PendingAdjustments.Add("📉 Quality degrading — consider changing worker models or refining the goal.");
18161821
}
@@ -1828,7 +1833,8 @@ private async Task SendViaOrchestratorReflectAsync(string groupId, List<string>
18281833
AddOrchestratorSystemMessage(orchestratorName, $"✅ {reflectState.BuildCompletionSummary()}");
18291834
break;
18301835
}
1831-
else if (synthesisResponse.Contains("[[GROUP_REFLECT_COMPLETE]]", StringComparison.OrdinalIgnoreCase)
1836+
1837+
if (synthesisResponse.Contains("[[GROUP_REFLECT_COMPLETE]]", StringComparison.OrdinalIgnoreCase)
18321838
&& !allWorkersDispatched)
18331839
{
18341840
// Override premature completion — not all workers have participated
@@ -1840,16 +1846,17 @@ private async Task SendViaOrchestratorReflectAsync(string groupId, List<string>
18401846
$"🔄 Overriding completion — {string.Join(", ", missing)} haven't participated yet.");
18411847
reflectState.RecordEvaluation(reflectState.CurrentIteration, 0.3,
18421848
reflectState.LastEvaluation, GetEffectiveModel(orchestratorName));
1843-
continue;
18441849
}
1850+
else
1851+
{
1852+
// Extract evaluation for next iteration
1853+
reflectState.LastEvaluation = ExtractIterationEvaluation(synthesisResponse);
18451854

1846-
// Extract evaluation for next iteration
1847-
reflectState.LastEvaluation = ExtractIterationEvaluation(synthesisResponse);
1848-
1849-
// Record a self-eval score (estimated from sentinel presence)
1850-
var selfScore = synthesisResponse.Contains("[[NEEDS_ITERATION]]", StringComparison.OrdinalIgnoreCase) ? 0.4 : 0.7;
1851-
reflectState.RecordEvaluation(reflectState.CurrentIteration, selfScore,
1852-
reflectState.LastEvaluation ?? "", GetEffectiveModel(orchestratorName));
1855+
// Record a self-eval score (estimated from sentinel presence)
1856+
var selfScore = synthesisResponse.Contains("[[NEEDS_ITERATION]]", StringComparison.OrdinalIgnoreCase) ? 0.4 : 0.7;
1857+
reflectState.RecordEvaluation(reflectState.CurrentIteration, selfScore,
1858+
reflectState.LastEvaluation ?? "", GetEffectiveModel(orchestratorName));
1859+
}
18531860
}
18541861

18551862
// Auto-adjustment: analyze worker results and suggest/apply changes

0 commit comments

Comments
 (0)