fix: don't promote URL-based groups to local folder groups#648
fix: don't promote URL-based groups to local folder groups#648
Conversation
ReconcileOrganization was converting URL-based groups to local folder groups when external worktrees existed, causing the '3 maui groups' bug: 1. Promotion hijacked the URL group (setting LocalPath) 2. Sessions on managed worktrees got migrated to a new URL group 3. Multiple external worktrees each triggered promotion, creating more groups Fix: never promote URL-based groups. Instead, create a separate local folder group for each external worktree path via GetOrCreateLocalFolderGroup. Also skip stale external worktree entries whose paths no longer exist on disk. Tests updated to match new behavior + new tests for: - Multiple external worktrees creating separate groups per path - Stale paths being skipped - Sessions staying in URL groups untouched Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔍 Expert Code Review — Design-Level Findings3 independent reviewers with adversarial consensus 🟡 MODERATE — Test passes for wrong reason (false-green risk) — Flagged by 2/3 reviewersFile: The test constructs All sibling tests were updated to use GUID-suffixed paths with Suggested fix: Add 🟢 MINOR — Stale comment references removed behavior — Flagged by 3/3 reviewersFile: The comment reads: "This fixes state from before the promotion migration was added." This PR removes the promotion migration from Suggested fix: Update to: "This heals sessions stranded in local folder groups whose worktree path doesn't match the group's LocalPath (e.g., sessions on managed worktrees that ended up in a local folder group due to legacy promotion or explicit folder addition)." Discarded Findings (single-reviewer only, not confirmed by follow-up)
Warning
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Code Review Summary
PR: fix: don't promote URL-based groups to local folder groups
Methodology: 3 independent reviewers with adversarial consensus (initial review + targeted follow-up verification)
Findings (ranked by severity)
| # | Severity | Finding | Consensus | Location |
|---|---|---|---|---|
| 1 | 🟡 MODERATE | Test NestedWorktreeSession_StaysInUrlGroup passes for wrong reason — missing Directory.CreateDirectory causes the Directory.Exists guard to skip the code path entirely, making the test a false green |
2/3 reviewers | Test file (outside diff) |
| 2 | 🟡 MODERATE | GetOrCreateLocalFolderGroup fires OnStateChanged mid-reconciliation before the session-healing loop runs, potentially rendering partial state in the UI |
2/3 reviewers (1 disagreed, noting most callers fire OnStateChanged afterward) |
Production code, line 750 (inline comment) |
| 3 | 🟢 MINOR | Stale comment at line 757 references "promotion migration" which this PR removes | 3/3 reviewers | Production code (outside diff) |
Overall Assessment
The core production code change is well-designed and correct. Replacing the promotion-based approach with separate local folder group creation is a clean solution to the "3 maui groups" bug. The new Directory.Exists guard for stale paths is a good defensive addition.
Test coverage is strong — 2 new tests (MultipleExternalWorktrees_SameRepo_CreatesGroupPerPath and StaleExternalWorktree_SkippedWhenPathNotOnDisk) cover important edge cases. The one false-green test (NestedWorktreeSession_StaysInUrlGroup) should be hardened with Directory.CreateDirectory to match the pattern in all sibling tests.
CI status: Tests pass (19/19 organization tests verified by reviewers).
Net code reduction: ~33 lines of complex promotion + session migration logic removed, replaced by ~6 lines of straightforward group creation. This is a significant simplification that reduces future maintenance burden.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
192.0.2.1
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "192.0.2.1"See Network Configuration for more information.
Generated by Expert Code Review (auto) for issue #648
|
|
||
| // Create a dedicated local folder group for this external path. | ||
| // This never touches the existing URL-based group. | ||
| GetOrCreateLocalFolderGroup(normalizedExtPath, ext.RepoId); |
There was a problem hiding this comment.
🟡 MODERATE — Mid-reconcile OnStateChanged fires before session healing (Flagged by 2/3 reviewers)
GetOrCreateLocalFolderGroup internally fires OnStateChanged?.Invoke() (line ~1504), which triggers a UI re-render before the session-healing loop at lines 758–791 has run. This means the UI could briefly render partially-reconciled state (new local folder group exists, but stranded sessions haven't been healed yet).
Counterargument (1/3 disagreed): Most callers of ReconcileOrganization fire OnStateChanged afterward, so the final state is delivered in the same UI batch. However, not all call sites necessarily follow this pattern.
Suggested mitigation: Consider adding a suppressNotify parameter to GetOrCreateLocalFolderGroup when called from ReconcileOrganization, or ensure the outer reconciliation always fires OnStateChanged at the end when changed is true.
Agents should run PolyPilot/relaunch.sh from their worktree, not from the primary checkout at ~/Projects/AutoPilot/PolyPilot/. The script uses dirname to resolve its build directory, so each worktree's copy builds its own code automatically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ PR #648 — Final Multi-Model Re-Review (Round 3)Re-review after all fix commits ( Previous Findings — All Resolved
6/6 fully fixed. Round 3 New FindingsNo findings reached 2/3 consensus. Discarded single-reviewer findings:
CI StatusTest Coverage✅ All 3506 tests pass. 8 tests directly cover the changed behavior including the new sibling-path regression test. Recommendation✅ Approve and merge — All findings resolved across 3 review rounds. Core design (no-promotion → separate local folder groups) is clean and well-tested. 3 independent reviewers · Adversarial consensus · Re-review round 3 |
1. Fix false-green NestedWorktreeSession test: add Directory.CreateDirectory with GUID suffix + try/finally cleanup + Assert.False(IsLocalFolder) 2. Path-aware auto-assignment: both sites (~579, ~612) now prefer the local folder group whose LocalPath matches the session's worktree path, with fallback to any local folder group for backward compatibility 3. Update stale comment at healing loop to reference legacy promotion and explicit folder addition instead of removed promotion migration 4. Suppress mid-reconciliation SaveOrganization: add skipNotify parameter to GetOrCreateLocalFolderGroup, used by ReconcileOrganization to batch saves instead of N+1 disk writes 5. Add IsInitialized to StaleExternalWorktree test for consistency with all sibling tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both auto-assignment sites used bare StartsWith without a trailing Path.DirectorySeparatorChar, causing /maui to false-match /maui2. Now mirrors the correct pattern from the heal-stranded-sessions code: normalized paths + separator suffix + exact-match fallback. Added AutoAssignment_SiblingPaths_DoesNotCrossMatch test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add clarifying comments on auto-assignment fallback explaining dependency on heal-stranded-sessions block. Fix TrimEnd(DirectorySeparatorChar) → TrimEnd(DirectorySeparatorChar, AltDirectorySeparatorChar) in test for Windows consistency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Problem
ReconcileOrganizationwas converting URL-based groups to local folder groups when external worktrees existed, causing the "3 maui groups" bug:LocalPathon the main group that had 16+ sessions on managed worktreesGetOrCreateRepoGroupwas called to house the displaced sessionsRoot Cause
The external worktree promotion loop (lines 710-780) found each external worktree path, then
promotedthe most-recently-created URL-based group by setting itsLocalPath. This was wrong when the URL group had sessions using managed worktrees (~/.polypilot/worktrees/...).Fix
Never promote URL-based groups. Instead:
GetOrCreateLocalFolderGroupDirectory.Existscheck)Tests
ReconcileOrganization_MultipleExternalWorktrees_SameRepo_CreatesGroupPerPathReconcileOrganization_StaleExternalWorktree_SkippedWhenPathNotOnDiskFixes the recurring "multiple maui groups" issue that kept coming back after PR #638.