Conversation
Previously, messages with image attachments sent to orchestrator sessions bypassed SendToMultiAgentGroupAsync entirely, going directly through SendPromptAsync. The orchestrator would produce @worker blocks in its response, but nobody parsed them — workers were never dispatched. Changes: - Dashboard.razor: Always route orchestrator messages through the dispatch pipeline. Images are stripped (pipeline doesn't support forwarding them) with a user-visible warning. This ensures @worker blocks are always parsed and workers are dispatched. - Dashboard.razor: Dispatch errors now logged via LogDispatchError (persisted to event-diagnostics.log) instead of Console.WriteLine (invisible). - CopilotService.Organization.cs: Added LogDispatchError method for persisting dispatch failures to diagnostics log. - CopilotService.Organization.cs: ResumeOrchestrationIfPendingAsync now resets stale ReflectionState (IsActive=false, CompletedAt set) after completing resume synthesis. Without this, StartGroupReflection returns early on the next user prompt because the old IsActive=true persists across app restarts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
🔍 Multi-Model Code Review — PR #590 (Final — Round 5)fix: route orchestrator messages through dispatch pipeline even with image attachments CI Status
Complete Finding History (Rounds 1–5)
Round 5 AssessmentWhat's New in v5
Verified Correct
No New Issues FoundAll 9 findings across 5 rounds of review are resolved. Recommendation✅ Approve This PR has been through 5 rounds of iterative multi-model review, addressing 9 findings spanning threading safety, race conditions, DateTime correctness, test coverage, error handling, temp file management, and API consistency. The implementation is thorough and well-tested. |
- Wrap ReflectionState reset in InvokeOnUI to match UI-thread-only convention (fixes threading bug and TOCTOU race) - Update image-strip warning to accurately communicate total image loss - Capture group name as local before async closure (null safety) - Add 2 behavioral tests for ReflectionState reset after resume Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The InvokeOnUI callback could reset a fresh ReflectionState created by a user prompt between ClearPendingOrchestration() and the callback firing. Now captures PendingOrchestration.StartedAt and compares it to the cycle's StartedAt — a fresh cycle (newer timestamp) is left untouched. Added test: ReflectionState_FreshCycleNotResetByStaleResume Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PendingOrchestration.StartedAt uses DateTime.UtcNow while ReflectionCycle.StartedAt uses DateTime.Now. In UTC-negative timezones (all Americas), the raw comparison incorrectly kills fresh cycles. Apply the existing ToLocalTime() pattern from line 3091-3093. Updated test to use DateTime.UtcNow for staleStartedAt to match the production clock source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…emp files Extract ClearPendingOrchestrationAndResetState() helper that combines ClearPendingOrchestration + ReflectionState reset + phase notification. Applied to all 6 ClearPendingOrchestration sites in the resume flow: - Group not found - Orchestrator session not found - Exception handler - Cancellation requested - No worker responses - Happy path (was inline, now uses helper) Also: delete temp image files when images are dropped for orchestrator dispatch, instead of leaking them until OS temp cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
added a commit
that referenced
this pull request
Apr 17, 2026
…#527) ## Problem When a user adds an existing repo via **Existing Folder** (e.g. their `dotnet/maui` clone), PolyPilot: 1. Created a **bare clone** of the entire repo at `~/.polypilot/repos/` (redundant - user already has it) 2. Created **nested worktree checkouts** inside the user's repo at `.polypilot/worktrees/{branch}/` 3. The user's **existing checkout was never used** for sessions For huge repos like MAUI (~5GB), this wasted significant disk space and time. Additionally, when both a URL-based repo and a local folder pointed to the same remote, both incorrectly used the same `BareClonePath`, causing the local folder to be ignored. ## Changes ### Skip bare clone for Existing Folder repos - `AddRepositoryFromLocalAsync` now points `BareClonePath` directly at the user's existing repo instead of creating a redundant bare clone - `EnsureRepoCloneInCurrentRootAsync` detects non-bare repos (`.git` dir/file) and skips clone management ### Separate repos for URL vs local folder - When a URL-based repo already exists, adding the same repo from a local folder creates a **separate** `RepositoryInfo` with a distinct ID (`{baseId}-local-{pathHash}`) - The original URL-based repo's `BareClonePath` is never overwritten - Idempotent: adding the same local folder twice returns the same repo ### Validate bare clone before reuse - Corrupt or partial bare clone directories are detected and removed before re-cloning ### Reuse existing checkout for same-branch sessions - `CreateWorktreeAsync` checks for an existing registered worktree on the requested branch before creating a new one - Only matches centralized worktrees (under `~/.polypilot/worktrees/`), never external user checkouts ### Remove nested worktree strategy - All worktrees now go to the centralized `~/.polypilot/worktrees/` directory - Removed `localPath` parameter from `CreateWorktreeAsync`, `CreateSessionWithWorktreeAsync`, and all UI callers ### Bug fixes - Fixed `Path.Combine` producing backslashes for Unix-style path in `BuildContinuationTranscript` - Fixed `FindActiveLockPid` process name filter to include `testhost` - Fixed repo picker to show full repo name instead of last dash-segment (#570) - Fixed existing repo and group name migration to URL-derived format (#570) ### Orchestrator image dispatch fix (cherry-picked from PR #590) - Orchestrator messages now always route through the dispatch pipeline even with image attachments - Previously, image attachments caused the dispatch pipeline to be bypassed entirely ## Testing All 3433 tests pass (12 pre-existing ExternalSessionScannerTests failures due to missing `node`). Test coverage includes: - 11 `AddExistingRepoTests` (local folder behavior, URL vs local separation, idempotency, validation errors, ID format, reconciliation) - 43 `RepoManagerTests` (URL/ID parsing, clone management, worktree reuse, git exclude entries) - 34 `WorktreeStrategyTests` (strategy behaviors, worktree creation) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a user sends a message to an orchestrator session that includes image attachments, the dispatch pipeline was completely bypassed. The message went directly to the orchestrator's SDK session via SendPromptAsync, and the orchestrator's response (containing @worker blocks) was never parsed or dispatched to workers.
Root Cause
Dashboard.razor line ~1922 had a guard that checked for empty imagePaths before routing through the dispatch pipeline. With images present, the code fell through to the direct send path, skipping SendToMultiAgentGroupAsync entirely.
Changes
Dashboard.razor
CopilotService.Organization.cs
Testing
All 3433 tests pass (12 pre-existing ExternalSessionScannerTests failures due to missing node).