feat: Intercept SDK task tool for multi-agent orchestration (#229)

[PureWeen]

Closes #229

What

Replaces fragile @worker: text-parsing with structured tool-call dispatch for multi-agent orchestration.

Changes

• Block SDK built-in task tool via ExcludedTools=["task"]
• Register custom task AIFunction that routes to workers via ExecuteWorkerAsync
• Thread-safe result collection (WorkerDelegationContext)
• Tool-dispatch path with @worker: text fallback
• Reconnect handling (clear _reflectToolConfigured)
• Codespace client support (GetClientForGroup)
• 14 unit tests

Testing

• 2114+ tests passing
• Use Implement & Challenge preset (OrchestratorReflect mode) to verify tool-dispatch
• Fallback to @worker: text parsing works when tool setup fails

[PureWeen]

PR #318 — 5-Model Consensus Review

Title: feat: Intercept SDK task tool for multi-agent orchestration
CI: ⚠️ No checks configured on feature/intercept-task-tool-229
Prior reviews: None
Models: claude-opus-4.6 ×2, gemini-3-pro-preview, gpt-5.3-codex (4/5 complete; 1 timed out — findings below reflect 4-model consensus)

---

🔴 CRITICAL — Reconnect-skip path leaves SendingFlag set → session deadlock (4/4 consensus)

PolyPilot/Services/CopilotService.cs — reconnect-skip block

The new orchestrator reconnect-skip path sets IsProcessing = false but does not call Interlocked.Exchange(ref state.SendingFlag, 0).

SendPromptAsync sets SendingFlag = 1 at entry. All other IsProcessing = false paths release it (either via CompleteResponse or explicit reset). This path does neither. The next call to SendPromptAsync for this session will fail the CompareExchange(ref state.SendingFlag, 1, 0) check and throw "Session is already processing a request" — permanently deadlocking the session until the app restarts.

Additional missing INV-1 cleanup in the same block (4/4 consensus):

• FlushCurrentResponse(state) not called before IsProcessing = false → any streaming content accumulated before reconnect is silently dropped
• ClearPermissionDenials() not called → stale permission denials persist into next turn
• OnSessionComplete not fired → external subscribers never unblocked

// Missing from the reconnect-skip block:
FlushCurrentResponse(state);                             // before IsProcessing = false
Interlocked.Exchange(ref state.SendingFlag, 0);          // ← CRITICAL: prevents deadlock
state.Info.ClearPermissionDenials();
OnSessionComplete?.Invoke(sessionName);
// Also missing diagnostic log tag: [RECONNECT-SKIP] or similar

---

🟡 MODERATE — OnStateChanged?.Invoke() in reconnect-skip path fires on background thread (4/4 consensus)

PolyPilot/Services/CopilotService.cs — same reconnect-skip block

OnStateChanged?.Invoke() is called directly without InvokeOnUI(). This code runs in SendPromptAsync's catch block, which for worker sessions executes on a Task.Run background thread. Project invariant INV-2 requires ALL IsProcessing state mutations and OnStateChanged notifications to be marshaled to the UI thread. Every other termination path uses InvokeOnUI() or _syncContext.Post(). This will cause cross-thread Blazor render exceptions under load.

Fix: Wrap the entire block in InvokeOnUI(() => { ... }).

---

🟡 MODERATE — Escalating poll calls CompleteResponse on background thread when _syncContext == null (4/4 consensus)

PolyPilot/Services/CopilotService.Organization.cs — EnsureOrchestratorToolsAsync, OnToolDispatchEnd lambda

if (_syncContext != null)
    _syncContext.Post(_ => CompleteResponse(cur, endGen), null);
else
    CompleteResponse(cur, endGen);  // ← called on Task.Run thread

The else branch calls CompleteResponse directly on the thread pool. CompleteResponse mutates IsProcessing, fires OnStateChanged, and clears SendingFlag — all must run on the UI thread per INV-2. The watchdog uses InvokeOnUI() (which handles null _syncContext safely); this code should too. In production MAUI _syncContext is typically non-null, but the fallback path is a correctness bug.

Fix: Replace else CompleteResponse(cur, endGen) with else InvokeOnUI(() => CompleteResponse(cur, endGen)).

---

🟡 MODERATE — AddPendingTask silently overwrites prior task for same worker name (3/4 consensus)

PolyPilot/Services/WorkerDelegationTool.cs — AddPendingTask

internal void AddPendingTask(string workerName, Task<ToolDispatchedResult> task)
{
    _pendingTasks[workerName] = task;  // overwrites silently
}

The round-robin index wraps around when all workers have been dispatched. With 2 workers and 3 dispatches, the sequence is w1→w2→w1. The second w1 dispatch overwrites the first task in _pendingTasks. The first w1 worker continues running in the background but its result is permanently lost — AwaitAllPendingAsync only collects the second task. This also produces an unobserved task exception if the first task faults.

Fix: Use a list-of-tasks keyed by worker name, or append a dispatch counter to the key.

---

🟡 MODERATE — Stale pending tasks from interrupted dispatch survive into new context (3/4 consensus)

PolyPilot/Services/CopilotService.Organization.cs — EnsureOrchestratorToolsAsync

var context = _delegationContexts.GetOrAdd(orchestratorName, _ => new WorkerDelegationContext());

GetOrAdd reuses the existing WorkerDelegationContext on reconnect. The orchestration loops call FullReset at the start of a new cycle (which clears _pendingTasks), but the reflect loop only calls Reset/ResetResults — which deliberately preserve pending tasks. After a reconnect mid-reflect-loop, the new iteration's EnsureOrchestratorToolsAsync reuses the context with surviving pending tasks from the interrupted dispatch. Those tasks are then awaited by AwaitAllPendingAsync in the new iteration, mixing stale results from the old dispatch with new ones.

---

🟡 MODERATE — NRE in synthesis session creation when _client is null (2/4 consensus)

PolyPilot/Services/CopilotService.Organization.cs — synthesis path

GitHub.Copilot.SDK.CopilotClient synthClient;
try { synthClient = GetClientForGroup(orchestratorGroupId2); }
catch { synthClient = _client; }
var freshSession = await synthClient.CreateSessionAsync(...);  // NRE if _client is null

If GetClientForGroup throws (codespace tunnel disconnected) AND _client is null (after a failed reconnect set _client = null; IsInitialized = false), synthClient is null and the next line throws NullReferenceException, crashing the synthesis phase with an opaque error.

Fix: catch { synthClient = _client ?? throw new InvalidOperationException("No client available for synthesis session"); }

---

Test Coverage Assessment

The 14 new tests in WorkerDelegationToolTests.cs are comprehensive for WorkerDelegationContext behavior (concurrency, timeouts, round-robin, nudge scenarios). The 6 new tests in MultiAgentRegressionTests.cs use the source-text-as-spec pattern (read source file, assert string patterns exist) which is fragile but consistent with the existing regression test style.

Missing test coverage for new code paths:

• Reconnect-skip path: no test verifying SendingFlag is cleared, FlushCurrentResponse called, and OnStateChanged fires on UI thread
• AddPendingTask overwrite: the existing Context_ConcurrentAddResult_DoesNotCorrupt test doesn't cover the overwrite scenario (same worker dispatched twice → second overwrites first)
• Stale pending task contamination after reconnect: no test for GetOrAdd reuse with surviving tasks

Suggested test cases:

1. ReconnectSkipPath_ClearsSendingFlag — verify SendingFlag == 0 after reconnect-skip fires
2. AddPendingTask_SameWorkerTwice_BothTasksAwaited — currently only the second task is collected
3. AwaitAllPendingAsync_StaleTasksFromPreviousCycle_AreNotIncluded

---

Summary

The thundering-herd fix (_clientReconnectLock) and GetClientForGroup usage in EnsureOrchestratorToolsAsync are correct. The WorkerDelegationContext thread-safety model is sound. The core issue is that the new reconnect-skip path introduces a new IsProcessing = false path that violates 4 of the project's 12 required cleanup steps — most critically, SendingFlag is not released, which will deadlock sessions.

⚠️ Request changes — address the CRITICAL SendingFlag issue + the two threading violations before merge.

---

4-model review: claude-opus-4.6 ×2, gemini-3-pro-preview, gpt-5.3-codex

[PureWeen]

PR #318 Review — feat: Intercept SDK task tool for multi-agent orchestration

CI Status: ⚠️ No CI checks configured on this branch
Existing reviews: None
Models: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex (5/5)
Consensus threshold: ≥2 models

---

🟡 MODERATE — Missing McpServers, SkillDirectories, SystemMessage in all fresh session configs (5/5 consensus)

Every SessionConfig created fresh in this PR omits MCP servers, skill directories, and system message. This affects five code paths:

Location	Affected path
CopilotService.Organization.cs	EnsureOrchestratorToolsAsync — forceCreate=true
CopilotService.Organization.cs	EnsureOrchestratorToolsAsync — resume path
CopilotService.Organization.cs	EnsureOrchestratorToolsAsync — "session not found" fallback
CopilotService.Organization.cs	SendViaOrchestratorAsync — synthesis session
CopilotService.Organization.cs	ExecuteWorkerAsync — dead-worker revival

Compare with the existing reconnect handler (CopilotService.cs:~2561) which correctly calls LoadMcpServers() and LoadSkillDirectories(). PR #330 fixed this exact gap for the standard reconnect path; this PR reintroduces it for five new paths.

The most impactful case is worker revival: a revived worker loses all external tool access — file editing, running tests, reading code — making the revival useless for any real task. The orchestrator forceCreate paths are less critical (orchestrators primarily call the task tool) but still lose custom system prompts and MCP context.

Fix: Copy the MCP/skills/system-message restoration pattern from the existing reconnect handler into each fresh SessionConfig construction.

---

🟡 MODERATE (disputed 2/5) — CompleteResponse called on threadpool when _syncContext is null

In OnToolDispatchEnd's background Task.Run, the fallback branch:

if (_syncContext != null)
    _syncContext.Post(_ => CompleteResponse(cur, endGen), null);
else
    CompleteResponse(cur, endGen);  // runs on threadpool

Per INV-2, all IsProcessing mutations must run on the UI thread. Two models (opus-4.6, gemini) flag this as an INV-2 violation. One model (sonnet) disagrees, arguing this is functionally identical to InvokeOnUI's null-context fallback behavior.

Additionally, the _syncContext.Post path dispatches CompleteResponse without a try-catch; all other dispatch sites use InvokeOnUI() or Invoke() which wrap the callback. An exception from CompleteResponse would propagate unhandled into the SynchronizationContext.

Suggestion: Use InvokeOnUI(() => CompleteResponse(cur, endGen)) to consolidate the dispatch logic and get the try-catch wrapping for free.

---

🟡 MODERATE (disputed 2/5) — Interlocked.Exchange(ref cur.ActiveToolCallCount, 0) races with concurrent ToolExecutionStartEvent

The polling loop unconditionally zeroes the count after ~3s:

Interlocked.Exchange(ref cur.ActiveToolCallCount, 0); // Force-reset
_syncContext.Post(_ => CompleteResponse(cur, endGen), null);

Two models (opus-4.6, gemini) flag that this races with a concurrent ToolExecutionStartEvent increment for a second parallel task call, potentially leaving ActiveToolCallCount at 0 while a second tool is in-flight.

One model (sonnet) disagrees: "The SDK invokes at most one is_override callback per turn, so OnToolDispatchStart/End fire exactly once before the 3-second poll runs." The comment in the code itself acknowledges this: "The SDK fires ToolExecutionStartEvent for ALL parallel tool calls but may only invoke our callback for ONE of them."

Given the competing analysis, this warrants confirmation of the SDK behavior but may not be a bug in practice.

---

Test Coverage Assessment

The PR includes 452-line WorkerDelegationToolTests.cs and 14 new regression tests in MultiAgentRegressionTests.cs. Strong coverage overall. Missing test cases:

1. MCP server propagation in revival/forceCreate paths: Verify that EnsureOrchestratorToolsAsync (forceCreate=true) and ExecuteWorkerAsync revival restore MCP servers in the new session config.
2. _syncContext == null dispatch path: Test that CompleteResponse is invoked correctly when _syncContext is null in the OnToolDispatchEnd fallback.

---

Below consensus (single-model findings, for author awareness)

• (sonnet) CopilotService.Events.cs:528 — CancelTurnEndFallback fires unconditionally before the ActiveToolCallCount guard. When the guard triggers (tools in-flight), both the SessionIdleEvent's CompleteResponse path AND the TurnEnd-fallback timer are gone — only the 3s OnToolDispatchEnd poll remains. Consider only cancelling CancelTurnEndFallback after the guard passes.
• (opus-1) CopilotService.Organization.cs:1182 — continuation retry calls retryCtx2.Reset(prompt, workerNames, ct) with the full workerNames list instead of remaining workers; round-robin restarts at worker-0 regardless of dispatch history.
• (opus-2) CopilotService.cs:2682 — non-volatile state.ActiveToolCallCount direct read in the reconnect path; all other reads use Volatile.Read.
• (opus-2) CopilotService.cs:2683-2698 — orchestrator reconnect-skip path sets IsProcessing=false without calling FlushCurrentResponse(state) first (per INV-1).
• (codex) WorkerDelegationContext.cs — _pendingTasks keyed only by workerName; dispatching the same worker twice in one iteration overwrites the first task handle, orphaning it.

---

Recommended Action

⚠️ Request changes

The MCP/skills regression (5/5 consensus) is the primary blocker: worker revival loses all external tool access, making the revival feature non-functional for any task requiring tools. The fix is mechanical — copy the MCP/skills restoration pattern from the existing reconnect handler into the five new SessionConfig construction sites.

[PureWeen]

Superseded by #339. The tool-intercept approach fought the SDK event model — reverting to enhanced text-parsing dispatch with JSON mode, retry loop, and ported reliability fixes.

[PureWeen]

PR #318 Review: "feat: Intercept SDK task tool for multi-agent orchestration"

CI Status: ⚠️ No CI checks configured

5 models reviewed (claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex). 4 consensus findings.

---

🔴 CRITICAL — [[GROUP_REFLECT_COMPLETE]] checked on wrong string (5/5 models)

CopilotService.Organization.cs — SendViaOrchestratorReflectAsync

The reflect loop termination check is performed on the concatenated WORKER responses, not on the orchestrator's synthesizing response. [[GROUP_REFLECT_COMPLETE]] is the signal the orchestrator sends when it decides the multi-agent loop is done — but the code checks the string that workers returned, not the orchestrator's reply. As a result, the reflect loop never self-terminates. It will run until the max iteration cap regardless of whether the orchestrator signals completion. This causes unbounded cost/latency on every multi-agent reflect cycle.

---

🔴 CRITICAL — OnToolDispatchEnd poll fires on wrong session after reconnect (4/5 models)

CopilotService.Organization.cs — EnsureOrchestratorToolsAsync / OnToolDispatchEnd

The poll callback captures endGen from the session's ProcessingGeneration at dispatch time. After a reconnect, the new session object starts at generation 0. If endGen was also 0 (first prompt after init), the endGen == state.Info.ProcessingGeneration guard is satisfied on the wrong session. CompleteResponse fires on the new/wrong session, incorrectly ending a prompt that hasn't finished.

---

🟡 MODERATE — FullReset() creates zombie tasks (4/5 models)

WorkerDelegationTool.cs — FullReset()

FullReset() clears _pendingTasks without first calling ObserveAllPending(). Any in-flight worker Task objects are abandoned with no observer. When they eventually complete or fault, they inject their (now-stale) results into the next orchestration cycle's task dictionary via the shared completion callback. This causes stale results from a previous cycle to corrupt the next cycle.

Fix: Call ObserveAllPending() (or equivalent drain) before Reset().

---

🟡 MODERATE — AddPendingTask duplicate key overwrites first task (3/5 models)

WorkerDelegationTool.cs — AddPendingTask()

Worker name is used as the dictionary key. In a reflect cycle where the same worker is dispatched more than once across iterations, the second AddPendingTask call for that worker silently overwrites the first task. The first worker's result is lost; the orchestrator never sees it. Combined with the FullReset() zombie issue, this creates unpredictable result sets in multi-iteration reflect cycles.

---

Test Coverage

No tests were added for the new WorkerDelegationTool, WorkerDelegationContext, or the orchestration interception path. Given the severity of the generation-guard race and the FullReset zombie issue, these are critical paths that need unit test coverage. Suggested:

• FullReset_DoesNotLeakPendingTaskResults
• AddPendingTask_DuplicateWorkerName_ThrowsOrDeduplicates
• OnToolDispatchEnd_DoesNotFireOnWrongGeneration

---

⚠️ Request changes — the [[GROUP_REFLECT_COMPLETE]] string check and OnToolDispatchEnd generation guard are both logic bugs that affect every multi-agent reflect cycle.