Skip to content

Commit 695a893

Browse files
author
Test
committed
fix(pr-clone): preserve detached HEAD state for rollback
getCurrentBranch() returns '' when HEAD is detached, which made cleanUp() short-circuit (leaving the created branch, cherry-picked commits, and auto-stash in place) and made persistState() refuse to write a crash-recovery record. Capture the detached commit SHA via the new GitExecutor.getHeadCommit() and restore/persist against it instead. Closes #204
1 parent fd3d513 commit 695a893

4 files changed

Lines changed: 214 additions & 8 deletions

File tree

src/common/git/gitExecutor.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,11 @@ export class GitExecutor {
617617
return stdout.trim();
618618
}
619619

620+
async getHeadCommit() {
621+
const { stdout } = await this.#execGitCommand(['rev-parse', 'HEAD']);
622+
return stdout.trim();
623+
}
624+
620625
async listRemotes(): Promise<Array<{ name: string; fetchUrl: string; pushUrl: string }>> {
621626
const { stdout } = await this.#execGitCommand(['remote', '-v']);
622627
const remotes = new Map<string, { name: string; fetchUrl: string; pushUrl: string }>();

src/services/prCloneInPlaceService.ts

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ import { PrCloneReportedError } from './prCloneError';
2121

2222
interface IServiceStore {
2323
originalBranch?: string;
24+
/** Ref to restore on cleanup: the original branch name, or the original commit SHA when HEAD was detached. */
25+
originalRef?: string;
26+
/** True when the clone was started from a detached HEAD (so `originalRef` is a SHA, not a branch). */
27+
isDetached?: boolean;
2428
createdBranchName?: string;
2529
stashMessage?: string;
2630
originalPrData?: PrCloneData;
@@ -37,6 +41,10 @@ export const PR_CLONE_IN_PLACE_STATE_KEY = 'gitSmartCheckout.prCloneInPlaceOpera
3741
export interface IPersistedCloneOperation {
3842
repoPath: string;
3943
originalBranch: string;
44+
/** Ref to restore on cleanup: the original branch name, or the original commit SHA when HEAD was detached. */
45+
originalRef: string;
46+
/** True when the clone was started from a detached HEAD (so `originalRef` is a SHA, not a branch). */
47+
isDetached: boolean;
4048
createdBranchName: string;
4149
stashMessage?: string;
4250
/** Shas that still need to land, including the one that may currently be mid-conflict. */
@@ -192,7 +200,8 @@ export class PrCloneInPlaceService extends PrCloneServiceBase {
192200

193201
// A service for an inactive clone mode can be disposed without ever
194202
// touching the repository. Do not reset that repository during cleanup.
195-
if (!this.serviceStore.originalBranch) {
203+
const restoreRef = this.serviceStore.originalBranch || this.serviceStore.originalRef;
204+
if (!restoreRef) {
196205
return;
197206
}
198207

@@ -224,12 +233,15 @@ export class PrCloneInPlaceService extends PrCloneServiceBase {
224233
let restoredOriginalBranch = false;
225234
if (!stayOnClonedBranch) {
226235
try {
227-
await this.git.checkout(this.serviceStore.originalBranch);
236+
if (this.serviceStore.isDetached) {
237+
// Detached HEAD has no branch to restore — check out the captured SHA directly.
238+
await this.git.checkout(restoreRef);
239+
} else {
240+
await this.git.checkout(this.serviceStore.originalBranch || restoreRef);
241+
}
228242
restoredOriginalBranch = true;
229243
} catch (error) {
230-
this.loggingService.warn(
231-
`Failed to restore original branch '${this.serviceStore.originalBranch}': ${error}`
232-
);
244+
this.loggingService.warn(`Failed to restore original ref '${restoreRef}': ${error}`);
233245
}
234246

235247
if (restoredOriginalBranch && this.serviceStore.stashMessage) {
@@ -299,6 +311,15 @@ export class PrCloneInPlaceService extends PrCloneServiceBase {
299311

300312
// Step 1: Store original branch and stash changes if needed
301313
this.serviceStore.originalBranch = await this.git.getCurrentBranch();
314+
if (!this.serviceStore.originalBranch) {
315+
// Detached HEAD: capture the commit SHA so cleanup can still restore the original
316+
// state instead of silently leaving the clone's branch/stash/commits behind.
317+
this.serviceStore.originalRef = await this.git.getHeadCommit();
318+
this.serviceStore.isDetached = true;
319+
} else {
320+
this.serviceStore.originalRef = this.serviceStore.originalBranch;
321+
this.serviceStore.isDetached = false;
322+
}
302323
updateProgress.report({ message: 'Checking for uncommitted changes...' });
303324

304325
const hasUncommittedChanges = await this.git.isWorkdirHasChanges();
@@ -455,14 +476,17 @@ export class PrCloneInPlaceService extends PrCloneServiceBase {
455476
return;
456477
}
457478

458-
const { originalBranch, createdBranchName, stashMessage, originalPrData } = this.serviceStore;
459-
if (!originalBranch || !createdBranchName || !originalPrData) {
479+
const { originalBranch, originalRef, isDetached, createdBranchName, stashMessage, originalPrData } =
480+
this.serviceStore;
481+
if ((!originalBranch && !originalRef) || !createdBranchName || !originalPrData) {
460482
return;
461483
}
462484

463485
const record: IPersistedCloneOperation = {
464486
repoPath: this.git.repositoryPath,
465-
originalBranch,
487+
originalBranch: originalBranch ?? '',
488+
originalRef: originalRef ?? originalBranch ?? '',
489+
isDetached: isDetached ?? false,
466490
createdBranchName,
467491
stashMessage,
468492
remainingShas: [...this.remainingShas],
@@ -488,6 +512,8 @@ export class PrCloneInPlaceService extends PrCloneServiceBase {
488512
private restoreServiceStoreFromRecord(record: IPersistedCloneOperation): void {
489513
this.serviceStore = {
490514
originalBranch: record.originalBranch,
515+
originalRef: record.originalRef,
516+
isDetached: record.isDetached,
491517
createdBranchName: record.createdBranchName,
492518
stashMessage: record.stashMessage,
493519
originalPrData: {
Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
import * as assert from 'assert';
2+
import { Memento } from 'vscode';
3+
4+
import { GitHubClient } from '../../common/api/ghClient';
5+
import { GitExecutor } from '../../common/git/gitExecutor';
6+
import {
7+
IPersistedCloneOperation,
8+
PR_CLONE_IN_PLACE_STATE_KEY,
9+
PrCloneInPlaceService,
10+
} from '../../services/prCloneInPlaceService';
11+
import { PrCloneData } from '../../services/prCloneService';
12+
import { GitHubPR } from '../../types/dataTypes';
13+
import { mockLogService } from '../e2e/helpers/mockLogService';
14+
15+
/**
16+
* Regression tests for issue 204: "Detached HEAD silently disables PR-clone rollback".
17+
*
18+
* When a PR clone is started from a detached HEAD, `getCurrentBranch()` returns `''`, which
19+
* used to make cleanUp() short-circuit (nothing restored, created branch/stash left behind)
20+
* and persistState() refuse to write a crash-recovery record. The fix captures the detached
21+
* commit SHA via `getHeadCommit()` and restores/persists against that instead.
22+
*/
23+
24+
function createFakeMemento(): Memento {
25+
const store = new Map<string, unknown>();
26+
27+
return {
28+
get: ((key: string, defaultValue?: unknown) =>
29+
store.has(key) ? store.get(key) : defaultValue) as Memento['get'],
30+
update: async (key: string, value: unknown) => {
31+
if (value === undefined) {
32+
store.delete(key);
33+
} else {
34+
store.set(key, value);
35+
}
36+
},
37+
keys: () => [...store.keys()],
38+
};
39+
}
40+
41+
const prData = {
42+
number: 204,
43+
title: 'Detached HEAD PR',
44+
body: 'desc',
45+
head: { ref: 'feature/source' },
46+
base: { ref: 'main' },
47+
labels: [],
48+
assignees: [],
49+
} as unknown as GitHubPR;
50+
51+
const cloneData: PrCloneData = {
52+
prData,
53+
targetBranch: 'main',
54+
featureBranch: 'feature/clone',
55+
description: 'desc',
56+
selectedCommits: ['c1'],
57+
isDraft: false,
58+
};
59+
60+
const DETACHED_SHA = 'abc1234deadbeefabc1234deadbeefabc1234de';
61+
62+
function createDetachedGitStub(repositoryPath = '/repo') {
63+
const calls = {
64+
reset: 0,
65+
checkout: [] as string[],
66+
popStash: [] as string[],
67+
deleteLocalBranch: [] as string[],
68+
cherryPickAbort: 0,
69+
isCherryPickInProgress: 0,
70+
};
71+
72+
const git = {
73+
repositoryPath,
74+
getCurrentBranch: async () => '',
75+
getHeadCommit: async () => DETACHED_SHA,
76+
isWorkdirHasChanges: async () => false,
77+
fetchPullRequestHead: async () => {},
78+
checkout: async (branch: string) => {
79+
calls.checkout.push(branch);
80+
},
81+
pullCurrentBranch: async () => {},
82+
createUniqueFeatureBranch: async () => 'feature/clone',
83+
commitExists: async () => true,
84+
hasConflicts: async () => false,
85+
cherryPick: async () => ({ conflicts: false }),
86+
isCherryPickInProgress: async () => {
87+
calls.isCherryPickInProgress += 1;
88+
return false;
89+
},
90+
reset: async () => {
91+
calls.reset += 1;
92+
},
93+
popStash: async (message: string) => {
94+
calls.popStash.push(message);
95+
},
96+
deleteLocalBranch: async (branch: string) => {
97+
calls.deleteLocalBranch.push(branch);
98+
},
99+
cherryPickAbort: async () => {
100+
calls.cherryPickAbort += 1;
101+
},
102+
pushBranchToGitHub: async () => {},
103+
} as unknown as GitExecutor;
104+
105+
return { git, calls };
106+
}
107+
108+
describe('PrCloneInPlaceService detached-HEAD rollback (issue 204)', () => {
109+
it('captures the HEAD SHA and persists state when started from detached HEAD', async () => {
110+
const memento = createFakeMemento();
111+
const { git } = createDetachedGitStub();
112+
// Stall on a conflict so we can inspect the persisted record before cleanup runs.
113+
(git as any).cherryPick = async () => ({ conflicts: true });
114+
const service = new PrCloneInPlaceService(git, {} as GitHubClient, mockLogService, memento);
115+
116+
await service.clonePR(cloneData);
117+
118+
const persisted = memento.get<IPersistedCloneOperation>(PR_CLONE_IN_PLACE_STATE_KEY);
119+
assert.ok(persisted, 'a record should be persisted even though originalBranch is empty');
120+
assert.strictEqual(persisted!.originalBranch, '', 'originalBranch stays empty for compatibility');
121+
assert.strictEqual(persisted!.originalRef, DETACHED_SHA, 'originalRef must capture the detached SHA');
122+
assert.strictEqual(persisted!.isDetached, true);
123+
});
124+
125+
it('cleanUp checks out the captured SHA (not a branch name) instead of silently returning', async () => {
126+
const memento = createFakeMemento();
127+
const { git, calls } = createDetachedGitStub();
128+
(git as any).cherryPick = async () => ({ conflicts: true });
129+
const service = new PrCloneInPlaceService(git, {} as GitHubClient, mockLogService, memento);
130+
131+
await service.clonePR(cloneData);
132+
// clonePR itself checks out the target branch and the new feature branch; only the
133+
// checkout(s) issued by cleanUp (below) are relevant to this assertion.
134+
calls.checkout.length = 0;
135+
136+
await service.abortClonePR();
137+
await new Promise((resolve) => setImmediate(resolve));
138+
139+
assert.strictEqual(calls.reset, 1, 'a started clone must still hard-reset on abort');
140+
assert.deepStrictEqual(
141+
calls.checkout,
142+
[DETACHED_SHA],
143+
'cleanup must restore by checking out the captured SHA, not a (nonexistent) branch name'
144+
);
145+
assert.deepStrictEqual(
146+
calls.deleteLocalBranch,
147+
['feature/clone'],
148+
'the created feature branch must still be deleted on abort'
149+
);
150+
assert.strictEqual(
151+
memento.get(PR_CLONE_IN_PLACE_STATE_KEY),
152+
undefined,
153+
'the persisted record must be cleared after abort'
154+
);
155+
});
156+
157+
it('cleanUp no longer short-circuits when originalBranch is empty but originalRef is set', async () => {
158+
const { git, calls } = createDetachedGitStub();
159+
const service = new PrCloneInPlaceService(git, {} as GitHubClient, mockLogService);
160+
161+
(service as any).serviceStore = {
162+
originalBranch: '',
163+
originalRef: DETACHED_SHA,
164+
isDetached: true,
165+
createdBranchName: 'feature/clone',
166+
};
167+
168+
await (service as any).cleanUp(true);
169+
170+
assert.deepStrictEqual(calls.checkout, [DETACHED_SHA]);
171+
assert.deepStrictEqual(calls.deleteLocalBranch, ['feature/clone']);
172+
});
173+
});

src/test/unit/prCloneInterruptedRecovery.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,8 @@ describe('checkForInterruptedPrClone activation flow', () => {
159159
return {
160160
repoPath: '/repo',
161161
originalBranch: 'original',
162+
originalRef: 'original',
163+
isDetached: false,
162164
createdBranchName: 'feature/clone',
163165
stashMessage: undefined,
164166
remainingShas: ['c2'],

0 commit comments

Comments
 (0)