Skip to content

Commit 472fd07

Browse files
WirasmWirasm
authored
feat(isolation): detect squash-merged and PR-merged branches in cleanup (#1027)
* Fix: detect squash-merged and PR-merged branches in isolation cleanup (#1026) `isolation cleanup --merged` only used `git branch --merged`, which misses squash-merged branches because the resulting commit has a different SHA. Bulk cleanup of task worktrees required a manual `gh pr list` per branch. Changes: - Add `isPatchEquivalent()` to `@archon/git` using `git cherry` to detect squash-merged branches - Add `getPrState()` to `@archon/isolation` for `gh`-based PR state lookup with per-invocation caching; soft-fails on missing gh / non-GitHub remotes - `cleanupMergedWorktrees()` now unions three signals (ancestry, patch equivalence, PR state); skips with a clear reason when PR is OPEN - Add `--include-closed` flag to `archon isolation cleanup --merged` to also remove worktrees whose PRs were closed without merging - Tests for all new code paths Fixes #1026 * fix: address review findings for squash-merge cleanup PR - branch.ts: add 'bad revision' to isPatchEquivalent expected errors so manually-deleted branches return false instead of throwing - pr-state.ts: add repoPath context to warn/debug log calls; capture ghStdout before try block to include in warn log for parse failures - pr-state.test.ts: remove redundant beforeEach reset (setupGhResponse already resets); add tests for non-ENOENT gh error and malformed JSON - cleanup-service.test.ts: add test for isPatchEquivalent unexpected throw → skipped with 'merge check failed' reason - isolation-operations.test.ts: add test for includeClosed: true forwarding through cleanupMergedEnvironments - docs: add --include-closed to all five affected docs (CLAUDE.md, reference/cli.md, book/isolation.md, book/quick-reference.md, getting-started/overview.md) - cli-internals.md: add isolation cleanup --merged flow diagram * simplify: remove redundant assignments and verbose filter in new code
1 parent dddff87 commit 472fd07

20 files changed

Lines changed: 677 additions & 17 deletions

File tree

‎CLAUDE.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,9 @@ bun run cli isolation cleanup 14 # Custom days
224224
# Clean up environments with branches merged into main (also deletes remote branches)
225225
bun run cli isolation cleanup --merged
226226

227+
# Also remove environments with closed (abandoned) PRs
228+
bun run cli isolation cleanup --merged --include-closed
229+
227230
# Validate workflow definitions and their referenced resources
228231
bun run cli validate workflows # All workflows
229232
bun run cli validate workflows my-workflow # Single workflow

‎packages/cli/src/cli.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,8 @@ async function main(): Promise<number> {
459459
// Check for --merged flag in remaining args
460460
const mergedFlag = args.includes('--merged') || positionals.includes('--merged');
461461
if (mergedFlag) {
462-
await isolationCleanupMergedCommand();
462+
const includeClosed = args.includes('--include-closed');
463+
await isolationCleanupMergedCommand({ includeClosed });
463464
} else {
464465
const days = parseInt(positionals[2] ?? '7', 10);
465466
await isolationCleanupCommand(days);

‎packages/cli/src/commands/isolation.test.ts‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* Tests for isolation complete command
33
*/
44
import { describe, it, expect, beforeEach, afterEach, mock, spyOn } from 'bun:test';
5-
import { isolationCompleteCommand } from './isolation';
5+
import { isolationCompleteCommand, isolationCleanupMergedCommand } from './isolation';
66

77
const mockLogger = {
88
fatal: mock(() => undefined),
@@ -44,6 +44,27 @@ mock.module('@archon/core/services/cleanup-service', () => ({
4444
cleanupMergedWorktrees: mockCleanupMergedWorktrees,
4545
}));
4646

47+
const mockListEnvironments = mock(() =>
48+
Promise.resolve({
49+
codebases: [
50+
{
51+
codebaseId: 'cb-1',
52+
defaultCwd: '/test/repo',
53+
repositoryUrl: 'https://github.com/owner/repo',
54+
environments: [],
55+
},
56+
],
57+
totalEnvironments: 0,
58+
ghostsReconciled: 0,
59+
})
60+
);
61+
const mockCleanupMergedEnvironments = mock(() => Promise.resolve({ removed: [], skipped: [] }));
62+
63+
mock.module('@archon/core/operations/isolation-operations', () => ({
64+
listEnvironments: mockListEnvironments,
65+
cleanupMergedEnvironments: mockCleanupMergedEnvironments,
66+
}));
67+
4768
const mockHasUncommittedChanges = mock(() => Promise.resolve(false));
4869
// Default: gh returns empty PR array, git log returns empty string (no commits to report)
4970
const mockExecFileAsync = mock((cmd: string) =>
@@ -358,3 +379,32 @@ describe('isolationCompleteCommand', () => {
358379
expect(consoleLogSpy).toHaveBeenCalledWith('\nComplete: 1 completed, 1 failed, 1 not found');
359380
});
360381
});
382+
383+
describe('isolationCleanupMergedCommand', () => {
384+
let consoleLogSpy: ReturnType<typeof spyOn>;
385+
let consoleErrorSpy: ReturnType<typeof spyOn>;
386+
387+
beforeEach(() => {
388+
consoleLogSpy = spyOn(console, 'log').mockImplementation(() => {});
389+
consoleErrorSpy = spyOn(console, 'error').mockImplementation(() => {});
390+
mockCleanupMergedEnvironments.mockReset();
391+
mockCleanupMergedEnvironments.mockResolvedValue({ removed: [], skipped: [] });
392+
});
393+
394+
afterEach(() => {
395+
consoleLogSpy.mockRestore();
396+
consoleErrorSpy.mockRestore();
397+
});
398+
399+
it('passes includeClosed=true when --include-closed flag is set', async () => {
400+
await isolationCleanupMergedCommand({ includeClosed: true });
401+
expect(mockCleanupMergedEnvironments).toHaveBeenCalledWith('cb-1', '/test/repo', {
402+
includeClosed: true,
403+
});
404+
});
405+
406+
it('defaults to includeClosed=false', async () => {
407+
await isolationCleanupMergedCommand();
408+
expect(mockCleanupMergedEnvironments).toHaveBeenCalledWith('cb-1', '/test/repo', {});
409+
});
410+
});

‎packages/cli/src/commands/isolation.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,9 @@ export async function isolationCleanupCommand(daysStale = 7): Promise<void> {
124124
* Cleanup merged isolation environments (branches merged into main)
125125
* Also deletes remote branches for merged environments
126126
*/
127-
export async function isolationCleanupMergedCommand(): Promise<void> {
127+
export async function isolationCleanupMergedCommand(
128+
options: { includeClosed?: boolean } = {}
129+
): Promise<void> {
128130
console.log('Finding environments with branches merged into main...');
129131

130132
const { codebases } = await listEnvironments();
@@ -141,7 +143,11 @@ export async function isolationCleanupMergedCommand(): Promise<void> {
141143
try {
142144
console.log(`\nChecking ${codebase.repositoryUrl ?? codebase.defaultCwd}...`);
143145

144-
const result = await cleanupMergedEnvironments(codebase.codebaseId, codebase.defaultCwd);
146+
const result = await cleanupMergedEnvironments(
147+
codebase.codebaseId,
148+
codebase.defaultCwd,
149+
options
150+
);
145151

146152
for (const branch of result.removed) {
147153
console.log(` Cleaned: ${branch}`);

‎packages/core/src/operations/isolation-operations.test.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ describe('cleanupMergedEnvironments', () => {
190190

191191
const result = await cleanupMergedEnvironments('cb-1', '/main');
192192

193-
expect(mockCleanupMerged).toHaveBeenCalledWith('cb-1', '/main');
193+
expect(mockCleanupMerged).toHaveBeenCalledWith('cb-1', '/main', {});
194194
expect(result.removed).toBe(2);
195195
});
196196

@@ -201,4 +201,12 @@ describe('cleanupMergedEnvironments', () => {
201201

202202
expect(result.errors).toEqual(['branch-a: git error']);
203203
});
204+
205+
test('forwards includeClosed option to cleanupMergedWorktrees', async () => {
206+
mockCleanupMerged.mockResolvedValueOnce({ removed: 1, errors: [] });
207+
208+
await cleanupMergedEnvironments('cb-1', '/main', { includeClosed: true });
209+
210+
expect(mockCleanupMerged).toHaveBeenCalledWith('cb-1', '/main', { includeClosed: true });
211+
});
204212
});

‎packages/core/src/operations/isolation-operations.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,8 @@ export async function cleanupStaleEnvironments(
167167
*/
168168
export async function cleanupMergedEnvironments(
169169
codebaseId: string,
170-
mainPath: string
170+
mainPath: string,
171+
options: { includeClosed?: boolean } = {}
171172
): Promise<CleanupOperationResult> {
172-
return cleanupMergedWorktrees(codebaseId, mainPath);
173+
return cleanupMergedWorktrees(codebaseId, mainPath, options);
173174
}

‎packages/core/src/services/cleanup-service.test.ts‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,15 @@ const mockHasUncommittedChanges = mock(() => Promise.resolve(false));
1212
const mockWorktreeExists = mock(() => Promise.resolve(false));
1313
const mockGetDefaultBranch = mock(() => Promise.resolve('main'));
1414
const mockIsBranchMerged = mock(() => Promise.resolve(false));
15+
const mockIsPatchEquivalent = mock(() => Promise.resolve(false));
1516
const mockGetLastCommitDate = mock(() => Promise.resolve(null as Date | null));
1617
mock.module('@archon/git', () => ({
1718
execFileAsync: mockExecFileAsync,
1819
hasUncommittedChanges: mockHasUncommittedChanges,
1920
worktreeExists: mockWorktreeExists,
2021
getDefaultBranch: mockGetDefaultBranch,
2122
isBranchMerged: mockIsBranchMerged,
23+
isPatchEquivalent: mockIsPatchEquivalent,
2224
getLastCommitDate: mockGetLastCommitDate,
2325
toRepoPath: (p: string) => p,
2426
toBranchName: (b: string) => b,
@@ -40,10 +42,13 @@ mock.module('../isolation', () => ({
4042
destroy: mockDestroy,
4143
}),
4244
}));
45+
type PrStateValue = 'MERGED' | 'CLOSED' | 'OPEN' | 'NONE';
46+
const mockGetPrState = mock(() => Promise.resolve('NONE' as PrStateValue));
4347
mock.module('@archon/isolation', () => ({
4448
getIsolationProvider: () => ({
4549
destroy: mockDestroy,
4650
}),
51+
getPrState: mockGetPrState,
4752
}));
4853

4954
// Mock isolation-environments DB
@@ -824,6 +829,10 @@ describe('cleanupMergedWorktrees', () => {
824829
// Reset defaults
825830
mockGetDefaultBranch.mockResolvedValue('main');
826831
mockIsBranchMerged.mockResolvedValue(false);
832+
mockIsPatchEquivalent.mockReset();
833+
mockIsPatchEquivalent.mockResolvedValue(false);
834+
mockGetPrState.mockReset();
835+
mockGetPrState.mockResolvedValue('NONE');
827836
mockHasUncommittedChanges.mockResolvedValue(false);
828837
mockWorktreeExists.mockResolvedValue(false);
829838
});
@@ -915,6 +924,164 @@ describe('cleanupMergedWorktrees', () => {
915924
reason: 'still used by 2 conversation(s)',
916925
});
917926
});
927+
928+
test('removes branch when git-cherry detects squash-merge', async () => {
929+
mockListByCodebase.mockResolvedValueOnce([
930+
{
931+
id: 'env-squash',
932+
branch_name: 'squash-branch',
933+
working_path: '/workspace/repo/worktrees/squash-branch',
934+
status: 'active',
935+
},
936+
]);
937+
mockIsBranchMerged.mockResolvedValueOnce(false);
938+
mockIsPatchEquivalent.mockResolvedValueOnce(true);
939+
mockGetConversationsUsingEnv.mockResolvedValueOnce([]);
940+
mockGetById.mockResolvedValueOnce({
941+
id: 'env-squash',
942+
working_path: '/workspace/repo/worktrees/squash-branch',
943+
status: 'active',
944+
});
945+
mockWorktreeExists.mockResolvedValueOnce(true);
946+
947+
const result = await cleanupMergedWorktrees('codebase-1', '/workspace/repo');
948+
949+
expect(result.removed).toContain('squash-branch');
950+
});
951+
952+
test('removes branch when PR is MERGED', async () => {
953+
mockListByCodebase.mockResolvedValueOnce([
954+
{
955+
id: 'env-pr-merged',
956+
branch_name: 'pr-merged-branch',
957+
working_path: '/workspace/repo/worktrees/pr-merged-branch',
958+
status: 'active',
959+
},
960+
]);
961+
mockIsBranchMerged.mockResolvedValueOnce(false);
962+
mockIsPatchEquivalent.mockResolvedValueOnce(false);
963+
mockGetPrState.mockResolvedValueOnce('MERGED');
964+
mockGetConversationsUsingEnv.mockResolvedValueOnce([]);
965+
mockGetById.mockResolvedValueOnce({
966+
id: 'env-pr-merged',
967+
working_path: '/workspace/repo/worktrees/pr-merged-branch',
968+
status: 'active',
969+
});
970+
mockWorktreeExists.mockResolvedValueOnce(true);
971+
972+
const result = await cleanupMergedWorktrees('codebase-1', '/workspace/repo');
973+
974+
expect(result.removed).toContain('pr-merged-branch');
975+
});
976+
977+
test('skips branch when PR is OPEN with clear reason', async () => {
978+
mockListByCodebase.mockResolvedValueOnce([
979+
{
980+
id: 'env-pr-open',
981+
branch_name: 'pr-open-branch',
982+
working_path: '/workspace/repo/worktrees/pr-open-branch',
983+
status: 'active',
984+
},
985+
]);
986+
mockIsBranchMerged.mockResolvedValueOnce(false);
987+
mockIsPatchEquivalent.mockResolvedValueOnce(false);
988+
mockGetPrState.mockResolvedValueOnce('OPEN');
989+
990+
const result = await cleanupMergedWorktrees('codebase-1', '/workspace/repo');
991+
992+
expect(result.removed).toHaveLength(0);
993+
expect(result.skipped).toContainEqual({
994+
branchName: 'pr-open-branch',
995+
reason: 'PR is open (active review)',
996+
});
997+
});
998+
999+
test('skips branch when PR is CLOSED and includeClosed=false', async () => {
1000+
mockListByCodebase.mockResolvedValueOnce([
1001+
{
1002+
id: 'env-pr-closed',
1003+
branch_name: 'pr-closed-branch',
1004+
working_path: '/workspace/repo/worktrees/pr-closed-branch',
1005+
status: 'active',
1006+
},
1007+
]);
1008+
mockIsBranchMerged.mockResolvedValueOnce(false);
1009+
mockIsPatchEquivalent.mockResolvedValueOnce(false);
1010+
mockGetPrState.mockResolvedValueOnce('CLOSED');
1011+
1012+
const result = await cleanupMergedWorktrees('codebase-1', '/workspace/repo');
1013+
1014+
expect(result.removed).toHaveLength(0);
1015+
});
1016+
1017+
test('removes branch when PR is CLOSED and includeClosed=true', async () => {
1018+
mockListByCodebase.mockResolvedValueOnce([
1019+
{
1020+
id: 'env-pr-closed-include',
1021+
branch_name: 'pr-closed-branch',
1022+
working_path: '/workspace/repo/worktrees/pr-closed-branch',
1023+
status: 'active',
1024+
},
1025+
]);
1026+
mockIsBranchMerged.mockResolvedValueOnce(false);
1027+
mockIsPatchEquivalent.mockResolvedValueOnce(false);
1028+
mockGetPrState.mockResolvedValueOnce('CLOSED');
1029+
mockGetConversationsUsingEnv.mockResolvedValueOnce([]);
1030+
mockGetById.mockResolvedValueOnce({
1031+
id: 'env-pr-closed-include',
1032+
working_path: '/workspace/repo/worktrees/pr-closed-branch',
1033+
status: 'active',
1034+
});
1035+
mockWorktreeExists.mockResolvedValueOnce(true);
1036+
1037+
const result = await cleanupMergedWorktrees('codebase-1', '/workspace/repo', {
1038+
includeClosed: true,
1039+
});
1040+
1041+
expect(result.removed).toContain('pr-closed-branch');
1042+
});
1043+
1044+
test('skips branch when no PR and not merged', async () => {
1045+
mockListByCodebase.mockResolvedValueOnce([
1046+
{
1047+
id: 'env-none',
1048+
branch_name: 'orphan-branch',
1049+
working_path: '/workspace/repo/worktrees/orphan-branch',
1050+
status: 'active',
1051+
},
1052+
]);
1053+
mockIsBranchMerged.mockResolvedValueOnce(false);
1054+
mockIsPatchEquivalent.mockResolvedValueOnce(false);
1055+
mockGetPrState.mockResolvedValueOnce('NONE');
1056+
1057+
const result = await cleanupMergedWorktrees('codebase-1', '/workspace/repo');
1058+
1059+
expect(result.removed).toHaveLength(0);
1060+
expect(result.skipped).toHaveLength(0);
1061+
});
1062+
1063+
test('skips branch when isPatchEquivalent throws unexpected error', async () => {
1064+
mockListByCodebase.mockResolvedValueOnce([
1065+
{
1066+
id: 'env-error',
1067+
branch_name: 'error-branch',
1068+
working_path: '/workspace/repo/worktrees/error-branch',
1069+
status: 'active',
1070+
},
1071+
]);
1072+
mockIsBranchMerged.mockResolvedValueOnce(false);
1073+
mockIsPatchEquivalent.mockRejectedValueOnce(new Error('permission denied'));
1074+
1075+
const result = await cleanupMergedWorktrees('codebase-1', '/workspace/repo');
1076+
1077+
expect(result.removed).toHaveLength(0);
1078+
expect(result.skipped).toContainEqual(
1079+
expect.objectContaining({
1080+
branchName: 'error-branch',
1081+
reason: expect.stringContaining('merge check failed'),
1082+
})
1083+
);
1084+
});
9181085
});
9191086

9201087
describe('onConversationClosed', () => {

0 commit comments

Comments
 (0)