Skip to content

fix: preserve group name when promoting to local folder group#607

Merged
PureWeen merged 2 commits intomainfrom
fix/preserve-group-name-on-promote
Apr 18, 2026
Merged

fix: preserve group name when promoting to local folder group#607
PureWeen merged 2 commits intomainfrom
fix/preserve-group-name-on-promote

Conversation

@PureWeen
Copy link
Copy Markdown
Owner

Problem

PromoteOrCreateLocalFolderGroup and ReconcileOrganization's external worktree migration both unconditionally renamed groups to Path.GetFileName(worktreePath) during promotion. This destroyed user-customized group names.

Repro: User has a group named "maui" for dotnet/maui sessions. They add ~/Projects/maui2 as an existing folder. The promote code finds the "maui" group (RepoId=dotnet-maui), sets LocalPath = ~/Projects/maui2, and renames it to "maui2" (Path.GetFileName("~/Projects/maui2")). The user's 17-session "maui" group is now called "maui2" and converted to a local folder group. A new orphan "maui" group is auto-created with only 1 session.

Root Cause

Two locations in CopilotService.Organization.cs overwrite the group name during promotion:

  1. PromoteOrCreateLocalFolderGroup (line 1536): candidate.Name = Path.GetFileName(normalized);
  2. ReconcileOrganization migration (line 744): groupToPromote.Name = Path.GetFileName(normalizedExtPath);

Fix

Remove the Name override in both locations. Promotion should only set LocalPath to convert the group to a local folder group — the user's chosen group name must be preserved.

Testing

  • 2 new regression tests: PreservesUserGroupName_WhenFolderNameDiffers and PreservesGroupName_WhenPromoting
  • All 8 promotion-related tests pass
  • Full suite: 3477 tests pass, 0 failures

PromoteOrCreateLocalFolderGroup and ReconcileOrganization's external
worktree migration both unconditionally renamed groups to
Path.GetFileName(worktreePath) during promotion. This destroyed
user-customized group names — e.g., a group named 'maui' was renamed
to 'maui2' because the local folder was ~/Projects/maui2.

Remove the Name override in both locations. Promotion should only set
LocalPath to convert the group to a local folder group, not change
the user's chosen name.

Adds two regression tests verifying name preservation in both the
explicit promote path and the ReconcileOrganization migration path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen
Copy link
Copy Markdown
Owner Author

PureWeen commented Apr 18, 2026

🔍 Multi-Model Code Review — PR #607

Reviewed by: 3 independent reviewers with adversarial consensus
Diff reviewed: +117/−4, 2 files (2 commits)
CI Status: ⚠️ No checks configured for this branch


v1 Findings — Status After Commit c9bcc64e

# Finding v1 v2
1 Empty-name fallback removed during promotion 🟢 FIXEDif (string.IsNullOrWhiteSpace(name)) guard added in both paths + test
2 Undocumented promotion vs creation naming asymmetry 🟢 FIXED — inline comment added at creation fallthrough + CreationPath_UsesFolderBasename test
3 Test paths don't exist on disk (implicit contract) 🟢 FIXED — "path need not exist on disk" comments added to both test methods

v2 Re-Review (commit c9bcc64e)

All 3 findings from v1 are resolved. The fix commit adds:

Production code (CopilotService.Organization.cs):

  • Defensive IsNullOrWhiteSpace fallback in both ReconcileOrganization and PromoteOrCreateLocalFolderGroup
  • 4-line comment documenting the intentional asymmetry at the creation-path fallthrough
  • Debug messages now include group name for better diagnostics

Tests (SessionOrganizationTests.cs):

  • CreationPath_UsesFolderBasename — pins the creation path behavior (folder basename used for new groups)
  • FallsBackToFolderName_WhenGroupNameIsEmpty — verifies corrupt empty-name groups are repaired
  • "Path need not exist on disk" notes on both existing regression tests

No new issues found in the fix commit. The changes are minimal, correct, and well-tested.


Recommended Action

✅ Approve

All findings addressed. 3479 tests pass, 0 failures. The fix correctly prevents group name destruction during promotion while maintaining defensive fallbacks for corrupt state.

…ests

- Add defensive fallback: if group has empty/whitespace name during
  promotion, fall back to Path.GetFileName (both code paths)
- Document intentional naming asymmetry between promotion (preserves
  existing name) and creation (uses folder basename) with inline comment
- Add test for creation path verifying folder basename is used
- Add test for empty-name fallback verifying repair behavior
- Add 'path need not exist on disk' notes to test comments

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen PureWeen merged commit fe31aa4 into main Apr 18, 2026
@PureWeen PureWeen deleted the fix/preserve-group-name-on-promote branch April 18, 2026 05:57
PureWeen added a commit that referenced this pull request Apr 18, 2026
## Problem

After PR #607 fixed group name destruction during promotion, a second
duplicate group issue appeared: `GetOrCreateRepoGroup` created a
URL-based group for repos that only exist as local folder repos (e.g.,
`dotnet-maui-local-66cccd41`). This caused two "maui" groups in the
sidebar — the local folder group (`maui3`) and a newly auto-created URL
group (`maui`).

## Root Cause

The "heal stranded sessions" loop in `ReconcileOrganization` calls
`GetOrCreateRepoGroup` for sessions in local folder groups whose
worktree paths are outside the group's `LocalPath`. For local-only repos
(IDs containing `-local-`), this created a redundant URL-based group
with the same name as the local folder group's repo name.

`GetOrCreateRepoGroup` skips `IsLocalFolder` groups when searching for
existing groups (by design — they coexist). But for repos that **only**
exist as local folder repos, this creates a duplicate.

## Fix

Add a guard in `GetOrCreateRepoGroup`: when a local folder group already
covers a repo with a `-local-` ID suffix, return `null` instead of
creating a redundant URL-based group. Repos without the `-local-` suffix
(i.e., repos with both URL and local entries) still allow URL group
creation for the heal-stranded-sessions scenario.

## Testing

- 1 new regression test:
`LocalOnlyRepo_DoesNotCreateUrlGroup_ForCentralizedWorktree`
- Existing `HealsStrandedSessions` tests still pass (URL repos with both
local + managed worktrees)
- Full suite: 3480 tests pass, 0 failures

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant