Skip to content

Commit 1e588f3

Browse files
jdaltonjdalton
committed
Extract branch cleanup logic into testable functions with unit tests
Refactors branch cleanup code into dedicated functions to improve testability: - branch-cleanup.mts: New module with 4 extracted cleanup functions - cleanupStaleBranch: Handles stale branches without PRs - cleanupFailedPrBranches: Cleans up after PR creation failures - cleanupSuccessfulPrLocalBranch: Removes local branch after PR success - cleanupErrorBranches: Handles unexpected errors in catch block - branch-cleanup.test.mts: 8 unit tests covering all cleanup scenarios - coana-fix.mts: Updated to use extracted functions All tests pass. Functions maintain same behavior with improved testability.
1 parent 63a487d commit 1e588f3

File tree

3 files changed

+269
-43
lines changed

3 files changed

+269
-43
lines changed

‎src/commands/fix/branch-cleanup.mts‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/**
2+
* Branch cleanup utilities for socket fix command.
3+
* Manages local and remote branch lifecycle during PR creation.
4+
*
5+
* Critical distinction: Remote branches are sacred when a PR exists, disposable when they don't.
6+
*/
7+
8+
import { debugFn } from '@socketsecurity/registry/lib/debug'
9+
import { logger } from '@socketsecurity/registry/lib/logger'
10+
11+
import { gitDeleteBranch, gitDeleteRemoteBranch } from '../../utils/git.mts'
12+
13+
/**
14+
* Clean up a stale branch (both remote and local).
15+
* Safe to delete both since no PR exists for this branch.
16+
*
17+
* Returns true if cleanup succeeded or should continue, false if should skip GHSA.
18+
*/
19+
export async function cleanupStaleBranch(
20+
branch: string,
21+
ghsaId: string,
22+
cwd: string,
23+
): Promise<boolean> {
24+
logger.warn(`Stale branch ${branch} found without open PR, cleaning up...`)
25+
debugFn('notice', `cleanup: deleting stale branch ${branch}`)
26+
27+
const deleted = await gitDeleteRemoteBranch(branch, cwd)
28+
if (!deleted) {
29+
logger.error(
30+
`Failed to delete stale remote branch ${branch}, skipping ${ghsaId}.`,
31+
)
32+
debugFn('error', `cleanup: remote deletion failed for ${branch}`)
33+
return false
34+
}
35+
36+
// Clean up local branch too to avoid conflicts.
37+
await gitDeleteBranch(branch, cwd)
38+
return true
39+
}
40+
41+
/**
42+
* Clean up branches after PR creation failure.
43+
* Safe to delete both remote and local since no PR was created.
44+
*/
45+
export async function cleanupFailedPrBranches(
46+
branch: string,
47+
cwd: string,
48+
): Promise<void> {
49+
// Clean up pushed branch since PR creation failed.
50+
// Safe to delete both remote and local since no PR exists.
51+
await gitDeleteRemoteBranch(branch, cwd)
52+
await gitDeleteBranch(branch, cwd)
53+
}
54+
55+
/**
56+
* Clean up local branch after successful PR creation.
57+
* Keeps remote branch - PR needs it to be mergeable.
58+
*/
59+
export async function cleanupSuccessfulPrLocalBranch(
60+
branch: string,
61+
cwd: string,
62+
): Promise<void> {
63+
// Clean up local branch only - keep remote branch for PR merge.
64+
await gitDeleteBranch(branch, cwd)
65+
}
66+
67+
/**
68+
* Clean up branches in catch block after unexpected error.
69+
* Safe to delete both remote and local since no PR was created.
70+
*/
71+
export async function cleanupErrorBranches(
72+
branch: string,
73+
cwd: string,
74+
remoteBranchExists: boolean,
75+
): Promise<void> {
76+
// Clean up remote branch if it exists (push may have succeeded before error).
77+
// Safe to delete both remote and local since no PR was created.
78+
if (remoteBranchExists) {
79+
await gitDeleteRemoteBranch(branch, cwd)
80+
}
81+
await gitDeleteBranch(branch, cwd)
82+
}

‎src/commands/fix/branch-cleanup.test.mts‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'
2+
3+
import {
4+
cleanupErrorBranches,
5+
cleanupFailedPrBranches,
6+
cleanupStaleBranch,
7+
cleanupSuccessfulPrLocalBranch,
8+
} from './branch-cleanup.mts'
9+
10+
const mockLogger = vi.hoisted(() => ({
11+
error: vi.fn(),
12+
warn: vi.fn(),
13+
}))
14+
15+
const mockDebugFn = vi.hoisted(() => vi.fn())
16+
17+
const mockGitDeleteBranch = vi.hoisted(() => vi.fn())
18+
const mockGitDeleteRemoteBranch = vi.hoisted(() => vi.fn())
19+
20+
vi.mock('@socketsecurity/registry/lib/logger', () => ({
21+
logger: mockLogger,
22+
}))
23+
24+
vi.mock('@socketsecurity/registry/lib/debug', () => ({
25+
debugFn: mockDebugFn,
26+
}))
27+
28+
vi.mock('../../utils/git.mts', () => ({
29+
gitDeleteBranch: mockGitDeleteBranch,
30+
gitDeleteRemoteBranch: mockGitDeleteRemoteBranch,
31+
}))
32+
33+
describe('branch-cleanup', () => {
34+
beforeEach(() => {
35+
vi.clearAllMocks()
36+
mockGitDeleteBranch.mockResolvedValue(true)
37+
mockGitDeleteRemoteBranch.mockResolvedValue(true)
38+
})
39+
40+
afterEach(() => {
41+
vi.clearAllMocks()
42+
})
43+
44+
describe('cleanupStaleBranch', () => {
45+
it('should return true and delete both branches when remote deletion succeeds', async () => {
46+
const result = await cleanupStaleBranch(
47+
'socket-fix/GHSA-test',
48+
'GHSA-test',
49+
'/test/repo',
50+
)
51+
52+
expect(result).toBe(true)
53+
expect(mockGitDeleteRemoteBranch).toHaveBeenCalledWith(
54+
'socket-fix/GHSA-test',
55+
'/test/repo',
56+
)
57+
expect(mockGitDeleteBranch).toHaveBeenCalledWith(
58+
'socket-fix/GHSA-test',
59+
'/test/repo',
60+
)
61+
expect(mockLogger.warn).toHaveBeenCalledWith(
62+
expect.stringContaining('Stale branch'),
63+
)
64+
})
65+
66+
it('should return false and skip local deletion when remote deletion fails', async () => {
67+
mockGitDeleteRemoteBranch.mockResolvedValue(false)
68+
69+
const result = await cleanupStaleBranch(
70+
'socket-fix/GHSA-test',
71+
'GHSA-test',
72+
'/test/repo',
73+
)
74+
75+
expect(result).toBe(false)
76+
expect(mockGitDeleteRemoteBranch).toHaveBeenCalledWith(
77+
'socket-fix/GHSA-test',
78+
'/test/repo',
79+
)
80+
expect(mockGitDeleteBranch).not.toHaveBeenCalled()
81+
expect(mockLogger.error).toHaveBeenCalledWith(
82+
expect.stringContaining('Failed to delete stale remote branch'),
83+
)
84+
})
85+
})
86+
87+
describe('cleanupFailedPrBranches', () => {
88+
it('should delete both remote and local branches', async () => {
89+
await cleanupFailedPrBranches('socket-fix/GHSA-test', '/test/repo')
90+
91+
expect(mockGitDeleteRemoteBranch).toHaveBeenCalledWith(
92+
'socket-fix/GHSA-test',
93+
'/test/repo',
94+
)
95+
expect(mockGitDeleteBranch).toHaveBeenCalledWith(
96+
'socket-fix/GHSA-test',
97+
'/test/repo',
98+
)
99+
})
100+
101+
it('should call functions in correct order (remote first, then local)', async () => {
102+
const calls: string[] = []
103+
mockGitDeleteRemoteBranch.mockImplementation(async () => {
104+
calls.push('remote')
105+
return true
106+
})
107+
mockGitDeleteBranch.mockImplementation(async () => {
108+
calls.push('local')
109+
return true
110+
})
111+
112+
await cleanupFailedPrBranches('socket-fix/GHSA-test', '/test/repo')
113+
114+
expect(calls).toEqual(['remote', 'local'])
115+
})
116+
})
117+
118+
describe('cleanupSuccessfulPrLocalBranch', () => {
119+
it('should only delete local branch', async () => {
120+
await cleanupSuccessfulPrLocalBranch('socket-fix/GHSA-test', '/test/repo')
121+
122+
expect(mockGitDeleteBranch).toHaveBeenCalledWith(
123+
'socket-fix/GHSA-test',
124+
'/test/repo',
125+
)
126+
expect(mockGitDeleteRemoteBranch).not.toHaveBeenCalled()
127+
})
128+
})
129+
130+
describe('cleanupErrorBranches', () => {
131+
it('should delete both remote and local when remote exists', async () => {
132+
await cleanupErrorBranches('socket-fix/GHSA-test', '/test/repo', true)
133+
134+
expect(mockGitDeleteRemoteBranch).toHaveBeenCalledWith(
135+
'socket-fix/GHSA-test',
136+
'/test/repo',
137+
)
138+
expect(mockGitDeleteBranch).toHaveBeenCalledWith(
139+
'socket-fix/GHSA-test',
140+
'/test/repo',
141+
)
142+
})
143+
144+
it('should only delete local when remote does not exist', async () => {
145+
await cleanupErrorBranches('socket-fix/GHSA-test', '/test/repo', false)
146+
147+
expect(mockGitDeleteRemoteBranch).not.toHaveBeenCalled()
148+
expect(mockGitDeleteBranch).toHaveBeenCalledWith(
149+
'socket-fix/GHSA-test',
150+
'/test/repo',
151+
)
152+
})
153+
154+
it('should call functions in correct order when remote exists', async () => {
155+
const calls: string[] = []
156+
mockGitDeleteRemoteBranch.mockImplementation(async () => {
157+
calls.push('remote')
158+
return true
159+
})
160+
mockGitDeleteBranch.mockImplementation(async () => {
161+
calls.push('local')
162+
return true
163+
})
164+
165+
await cleanupErrorBranches('socket-fix/GHSA-test', '/test/repo', true)
166+
167+
expect(calls).toEqual(['remote', 'local'])
168+
})
169+
})
170+
})

‎src/commands/fix/coana-fix.mts‎

Lines changed: 17 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@ import { readJsonSync } from '@socketsecurity/registry/lib/fs'
88
import { logger } from '@socketsecurity/registry/lib/logger'
99
import { pluralize } from '@socketsecurity/registry/lib/words'
1010

11+
import {
12+
cleanupErrorBranches,
13+
cleanupFailedPrBranches,
14+
cleanupStaleBranch,
15+
cleanupSuccessfulPrLocalBranch,
16+
} from './branch-cleanup.mts'
1117
import {
1218
checkCiEnvVars,
1319
getCiEnvInstructions,
@@ -359,25 +365,13 @@ export async function coanaFix(
359365

360366
// If branch exists but no open PR, delete the stale branch.
361367
// This handles cases where PR creation failed but branch was pushed.
362-
// Safe to delete both remote and local since no PR exists.
363368
// eslint-disable-next-line no-await-in-loop
364369
if (await gitRemoteBranchExists(branch, cwd)) {
365-
logger.warn(
366-
`Stale branch ${branch} found without open PR, cleaning up...`,
367-
)
368-
debugFn('notice', `cleanup: deleting stale branch ${branch}`)
369370
// eslint-disable-next-line no-await-in-loop
370-
const deleted = await gitDeleteRemoteBranch(branch, cwd)
371-
if (!deleted) {
372-
logger.error(
373-
`Failed to delete stale remote branch ${branch}, skipping ${ghsaId}.`,
374-
)
375-
debugFn('error', `cleanup: remote deletion failed for ${branch}`)
371+
const shouldContinue = await cleanupStaleBranch(branch, ghsaId, cwd)
372+
if (!shouldContinue) {
376373
continue ghsaLoop
377374
}
378-
// Clean up local branch too to avoid conflicts.
379-
// eslint-disable-next-line no-await-in-loop
380-
await gitDeleteBranch(branch, cwd)
381375
}
382376

383377
// Check for GitHub token before doing any git operations.
@@ -475,7 +469,7 @@ export async function coanaFix(
475469

476470
// Clean up local branch only - keep remote branch for PR merge.
477471
// eslint-disable-next-line no-await-in-loop
478-
await gitDeleteBranch(branch, cwd)
472+
await cleanupSuccessfulPrLocalBranch(branch, cwd)
479473
} else {
480474
// Handle PR creation failures.
481475
if (prResult.reason === 'already_exists') {
@@ -487,42 +481,26 @@ export async function coanaFix(
487481
logger.error(
488482
`Failed to create PR for ${ghsaId}:\n${prResult.details}`,
489483
)
490-
// Clean up pushed branch since PR creation failed.
491-
// Safe to delete both remote and local since no PR exists.
492-
// eslint-disable-next-line no-await-in-loop
493-
await gitDeleteRemoteBranch(branch, cwd)
494484
// eslint-disable-next-line no-await-in-loop
495-
await gitDeleteBranch(branch, cwd)
485+
await cleanupFailedPrBranches(branch, cwd)
496486
} else if (prResult.reason === 'permission_denied') {
497487
logger.error(
498488
`Failed to create PR for ${ghsaId}: Permission denied. Check SOCKET_CLI_GITHUB_TOKEN permissions.`,
499489
)
500-
// Clean up pushed branch since PR creation failed.
501-
// Safe to delete both remote and local since no PR exists.
502490
// eslint-disable-next-line no-await-in-loop
503-
await gitDeleteRemoteBranch(branch, cwd)
504-
// eslint-disable-next-line no-await-in-loop
505-
await gitDeleteBranch(branch, cwd)
491+
await cleanupFailedPrBranches(branch, cwd)
506492
} else if (prResult.reason === 'network_error') {
507493
logger.error(
508494
`Failed to create PR for ${ghsaId}: Network error. Please try again.`,
509495
)
510-
// Clean up pushed branch since PR creation failed.
511-
// Safe to delete both remote and local since no PR exists.
512-
// eslint-disable-next-line no-await-in-loop
513-
await gitDeleteRemoteBranch(branch, cwd)
514496
// eslint-disable-next-line no-await-in-loop
515-
await gitDeleteBranch(branch, cwd)
497+
await cleanupFailedPrBranches(branch, cwd)
516498
} else {
517499
logger.error(
518500
`Failed to create PR for ${ghsaId}: ${prResult.error.message}`,
519501
)
520-
// Clean up pushed branch since PR creation failed.
521-
// Safe to delete both remote and local since no PR exists.
522502
// eslint-disable-next-line no-await-in-loop
523-
await gitDeleteRemoteBranch(branch, cwd)
524-
// eslint-disable-next-line no-await-in-loop
525-
await gitDeleteBranch(branch, cwd)
503+
await cleanupFailedPrBranches(branch, cwd)
526504
}
527505
}
528506

@@ -536,19 +514,15 @@ export async function coanaFix(
536514
`Unexpected condition: Push failed for ${ghsaId}, skipping PR creation.`,
537515
)
538516
debugDir('error', e)
539-
// Clean up remote branch if it exists (push may have succeeded before error).
540-
// Safe to delete both remote and local since no PR was created.
517+
// Clean up branches (push may have succeeded before error).
541518
// eslint-disable-next-line no-await-in-loop
542-
if (await gitRemoteBranchExists(branch, cwd)) {
543-
// eslint-disable-next-line no-await-in-loop
544-
await gitDeleteRemoteBranch(branch, cwd)
545-
}
519+
const remoteBranchExists = await gitRemoteBranchExists(branch, cwd)
520+
// eslint-disable-next-line no-await-in-loop
521+
await cleanupErrorBranches(branch, cwd, remoteBranchExists)
546522
// eslint-disable-next-line no-await-in-loop
547523
await gitResetAndClean(fixEnv.baseBranch, cwd)
548524
// eslint-disable-next-line no-await-in-loop
549525
await gitCheckoutBranch(fixEnv.baseBranch, cwd)
550-
// eslint-disable-next-line no-await-in-loop
551-
await gitDeleteBranch(branch, cwd)
552526
}
553527

554528
count += 1

0 commit comments

Comments
 (0)