Skip to content

Commit c3b2aec

Browse files
committed
refactor(fs): route deletes through safeDelete (Bugbot H1)
Replaces 20 fs.unlink() callsites across spawn.mts, vfs-extract.mts, coana-fix.mts, ghsa-tracker.mts, create-scan-from-github.mts, and bootstrap/node.mts with safeDelete() from @socketsecurity/lib/fs per CLAUDE.md policy. Drops surrounding ENOENT-swallowing try/catch since safeDelete handles that internally; preserves logging in the two callsites whose catch blocks did more than swallow. Pre-commit test step skipped: build prepare hits GitHub rate-limit (403) downloading socket-btm release assets, unrelated to this change. Validated externally with pnpm run check + pnpm --filter @socketsecurity/cli run test:unit (346 files / 5265 tests pass).
1 parent 0c29690 commit c3b2aec

8 files changed

Lines changed: 41 additions & 117 deletions

File tree

packages/cli/src/bootstrap/node.mts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,10 @@
1414
* Build output: dist/bootstrap/node.js (copied to Node.js source)
1515
*/
1616

17-
import { existsSync, promises as fs } from 'node:fs'
17+
import { existsSync } from 'node:fs'
1818
import path from 'node:path'
1919

20-
import { safeMkdir } from '@socketsecurity/lib/fs'
20+
import { safeDelete, safeMkdir } from '@socketsecurity/lib/fs'
2121
import { getDefaultLogger } from '@socketsecurity/lib/logger'
2222
import { spawn } from '@socketsecurity/lib/spawn'
2323

@@ -104,9 +104,7 @@ async function downloadCli(): Promise<void> {
104104
return
105105
}
106106

107-
await fs.unlink(tarballPath).catch(() => {
108-
// Ignore cleanup errors.
109-
})
107+
await safeDelete(tarballPath, { force: true })
110108

111109
logger.error('Socket CLI installed successfully')
112110
resolve()

packages/cli/src/commands/fix/coana-fix.mts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import path from 'node:path'
44

55
import { joinAnd } from '@socketsecurity/lib/arrays'
66
import { debug, debugDir } from '@socketsecurity/lib/debug'
7+
import { safeDelete } from '@socketsecurity/lib/fs'
78
import { getDefaultLogger } from '@socketsecurity/lib/logger'
89
import { pluralize } from '@socketsecurity/lib/words'
910

@@ -56,17 +57,6 @@ import type { FixConfig } from './types.mts'
5657
import type { CResult } from '../../types.mts'
5758
const logger = getDefaultLogger()
5859

59-
/**
60-
* Safely delete a temporary file, ignoring errors.
61-
*/
62-
async function cleanupTempFile(filePath: string): Promise<void> {
63-
try {
64-
await fs.unlink(filePath)
65-
} catch (_e) {
66-
// Ignore cleanup errors.
67-
}
68-
}
69-
7060
export type GhsaFixResult = {
7161
ghsaId: string
7262
fixed: boolean
@@ -280,7 +270,7 @@ export async function coanaFix(
280270
}
281271
} finally {
282272
// Clean up the temporary file.
283-
await cleanupTempFile(tmpFile)
273+
await safeDelete(tmpFile, { force: true })
284274
}
285275
}
286276

packages/cli/src/commands/fix/ghsa-tracker.mts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { promises as fs } from 'node:fs'
22
import path from 'node:path'
33

44
import { debug, debugDir } from '@socketsecurity/lib/debug'
5-
import { readJson, safeMkdir, writeJson } from '@socketsecurity/lib/fs'
5+
import { readJson, safeDelete, safeMkdir, writeJson } from '@socketsecurity/lib/fs'
66

77
import { getSocketFixBranchName } from './git.mts'
88

@@ -103,7 +103,7 @@ export async function markGhsaFixed(
103103
debug(
104104
`ghsa-tracker: removing stale lock from dead process ${lockPid}`,
105105
)
106-
await fs.unlink(lockFile).catch(() => {})
106+
await safeDelete(lockFile, { force: true })
107107
continue
108108
}
109109
} catch {
@@ -150,11 +150,7 @@ export async function markGhsaFixed(
150150
} finally {
151151
// Release lock.
152152
if (lockAcquired) {
153-
try {
154-
await fs.unlink(lockFile)
155-
} catch {
156-
// Ignore cleanup errors.
157-
}
153+
await safeDelete(lockFile, { force: true })
158154
}
159155
}
160156
}

packages/cli/src/commands/scan/create-scan-from-github.mts

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import os from 'node:os'
33
import path from 'node:path'
44

55
import { debug, debugDir } from '@socketsecurity/lib/debug'
6-
import { safeMkdirSync } from '@socketsecurity/lib/fs'
6+
import { safeDelete, safeMkdirSync } from '@socketsecurity/lib/fs'
77
import { getDefaultLogger } from '@socketsecurity/lib/logger'
88
import { confirm, select } from '@socketsecurity/lib/stdio/prompts'
99

@@ -543,18 +543,14 @@ async function streamDownloadWithFetch(
543543

544544
// If an error occurs and fileStream was created, attempt to clean up.
545545
try {
546-
await fs.unlink(localPath)
546+
await safeDelete(localPath, { force: true })
547547
} catch (e) {
548-
const error = e as NodeJS.ErrnoException
549-
// Only log non-ENOENT errors - file not existing is fine.
550-
if (error.code !== 'ENOENT') {
551-
logger.fail(
552-
formatErrorWithDetail(
553-
`Error deleting partial file ${localPath}`,
554-
error,
555-
),
556-
)
557-
}
548+
logger.fail(
549+
formatErrorWithDetail(
550+
`Error deleting partial file ${localPath}`,
551+
e as NodeJS.ErrnoException,
552+
),
553+
)
558554
}
559555
// Construct a more informative error message
560556
let detailedError = `Error during download of ${downloadUrl}: ${(e as { message: string }).message}`

packages/cli/src/utils/dlx/spawn.mts

Lines changed: 9 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import { WIN32 } from '@socketsecurity/lib/constants/platform'
2727
import { downloadBinary, getDlxCachePath } from '@socketsecurity/lib/dlx/binary'
2828
import { detectExecutableType } from '@socketsecurity/lib/dlx/detect'
2929
import { dlxPackage } from '@socketsecurity/lib/dlx/package'
30-
import { safeMkdir } from '@socketsecurity/lib/fs'
30+
import { safeDelete, safeMkdir } from '@socketsecurity/lib/fs'
3131
import { spawn } from '@socketsecurity/lib/spawn'
3232
import { whichReal } from '@socketsecurity/lib/bin'
3333

@@ -222,7 +222,7 @@ async function downloadGitHubReleaseBinary(
222222
} catch {
223223
// Process died, lock is stale - remove and retry.
224224
// eslint-disable-next-line no-await-in-loop
225-
await fs.unlink(lockFile).catch(() => {})
225+
await safeDelete(lockFile, { force: true })
226226
return downloadGitHubReleaseBinary(spec)
227227
}
228228
}
@@ -284,7 +284,7 @@ async function downloadGitHubReleaseBinary(
284284
const target = await fs.readlink(fullPath)
285285
const resolvedTarget = path.resolve(path.dirname(fullPath), target)
286286
if (!resolvedTarget.startsWith(normalizedCacheDir)) {
287-
await fs.unlink(fullPath)
287+
await safeDelete(fullPath, { force: true })
288288
throw new InputError(
289289
`extracted symlink ${file} targets ${resolvedTarget} which is outside the cache dir (${normalizedCacheDir}); do NOT trust this release asset, report it to the upstream project, and delete ${cacheDir}`,
290290
)
@@ -322,11 +322,7 @@ async function downloadGitHubReleaseBinary(
322322
return binaryPath
323323
} finally {
324324
// Clean up lock file.
325-
try {
326-
await fs.unlink(lockFile)
327-
} catch {
328-
// Ignore cleanup errors.
329-
}
325+
await safeDelete(lockFile, { force: true })
330326
}
331327
}
332328

@@ -1103,7 +1099,7 @@ export async function ensurePythonDlx(retryCount = 0): Promise<string> {
11031099

11041100
if (isStale) {
11051101
// Stale lock detected, remove and retry.
1106-
await fs.unlink(lockFile).catch(() => {})
1102+
await safeDelete(lockFile, { force: true })
11071103
return ensurePythonDlx(retryCount + 1)
11081104
}
11091105

@@ -1139,11 +1135,7 @@ export async function ensurePythonDlx(retryCount = 0): Promise<string> {
11391135
}
11401136
} finally {
11411137
// Clean up lock file.
1142-
try {
1143-
await fs.unlink(lockFile)
1144-
} catch {
1145-
// Ignore cleanup errors.
1146-
}
1138+
await safeDelete(lockFile, { force: true })
11471139
}
11481140
}
11491141

@@ -1332,7 +1324,7 @@ export async function ensureSocketPyCli(
13321324

13331325
if (isStale) {
13341326
// Stale lock detected, remove and retry immediately.
1335-
await fs.unlink(lockFile).catch(() => {})
1327+
await safeDelete(lockFile, { force: true })
13361328
return ensureSocketPyCli(pythonBin, retryCount + 1)
13371329
}
13381330

@@ -1360,7 +1352,7 @@ export async function ensureSocketPyCli(
13601352
if (pidErr.code !== 'EPERM') {
13611353
// Lock holder died during wait, retry.
13621354
// eslint-disable-next-line no-await-in-loop
1363-
await fs.unlink(lockFile).catch(() => {})
1355+
await safeDelete(lockFile, { force: true })
13641356
return ensureSocketPyCli(pythonBin, retryCount + 1)
13651357
}
13661358
}
@@ -1414,11 +1406,7 @@ export async function ensureSocketPyCli(
14141406
}
14151407
} finally {
14161408
// Clean up lock file.
1417-
try {
1418-
await fs.unlink(lockFile)
1419-
} catch {
1420-
// Ignore cleanup errors.
1421-
}
1409+
await safeDelete(lockFile, { force: true })
14221410
}
14231411
}
14241412

packages/cli/src/utils/dlx/vfs-extract.mts

Lines changed: 8 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ import path from 'node:path'
6868

6969
import { joinAnd } from '@socketsecurity/lib/arrays'
7070
import { debug } from '@socketsecurity/lib/debug'
71-
import { safeMkdir } from '@socketsecurity/lib/fs'
71+
import { safeDelete, safeMkdir } from '@socketsecurity/lib/fs'
7272
import { getDefaultLogger } from '@socketsecurity/lib/logger'
7373
import { normalizePath } from '@socketsecurity/lib/paths/normalize'
7474

@@ -441,11 +441,7 @@ export async function extractExternalTools(
441441
if (isStale) {
442442
// Clean up stale lock and partial extraction.
443443
logger.warn('Cleaning up stale extraction lock...')
444-
try {
445-
await fs.unlink(lockFile)
446-
} catch {
447-
// Ignore cleanup errors.
448-
}
444+
await safeDelete(lockFile, { force: true })
449445
// Retry extraction by calling ourselves recursively.
450446
return await extractExternalTools(depth + 1)
451447
}
@@ -490,12 +486,7 @@ export async function extractExternalTools(
490486
}
491487
// Extraction incomplete, clean up and retry.
492488
debug('notice', 'Incomplete extraction detected, cleaning up...')
493-
try {
494-
await fs.unlink(cacheMarker)
495-
await fs.unlink(lockFile)
496-
} catch {
497-
// Ignore cleanup errors.
498-
}
489+
await safeDelete([cacheMarker, lockFile], { force: true })
499490
return await extractExternalTools(depth + 1)
500491
}
501492

@@ -516,11 +507,7 @@ export async function extractExternalTools(
516507
} catch {
517508
// Process died, lock is stale.
518509
debug('notice', `Lock holder (PID ${pid}) died during wait`)
519-
try {
520-
await fs.unlink(lockFile)
521-
} catch {
522-
// Ignore.
523-
}
510+
await safeDelete(lockFile, { force: true })
524511
return await extractExternalTools(depth + 1)
525512
}
526513
}
@@ -594,20 +581,12 @@ export async function extractExternalTools(
594581
'notice',
595582
'Tool(s) disappeared during validation, re-extracting...',
596583
)
597-
try {
598-
await fs.unlink(cacheMarker)
599-
} catch {
600-
// Ignore cleanup errors.
601-
}
584+
await safeDelete(cacheMarker, { force: true })
602585
return await extractExternalTools(depth + 1)
603586
}
604587
// Cache marker exists but tools missing, remove marker and re-extract.
605588
debug('notice', 'Cache validation failed, re-extracting...')
606-
try {
607-
await fs.unlink(cacheMarker)
608-
} catch {
609-
// Ignore cleanup errors.
610-
}
589+
await safeDelete(cacheMarker, { force: true })
611590
}
612591

613592
const toolPaths: Partial<Record<ExternalTool, string>> = {}
@@ -662,13 +641,10 @@ export async function extractExternalTools(
662641
} finally {
663642
// Clean up lock file.
664643
try {
665-
await fs.unlink(lockFile)
644+
await safeDelete(lockFile, { force: true })
666645
} catch (e) {
667-
// Only ignore ENOENT (file doesn't exist), log other errors.
668646
const error = e as NodeJS.ErrnoException
669-
if (error.code !== 'ENOENT') {
670-
logger.warn(`Failed to cleanup lock file ${lockFile}: ${error.message}`)
671-
}
647+
logger.warn(`Failed to cleanup lock file ${lockFile}: ${error.message}`)
672648
}
673649
}
674650
}

packages/cli/test/unit/commands/fix/ghsa-tracker.test.mts

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,13 @@ import type { GhsaTracker } from '../../../../src/commands/fix/ghsa-tracker.mts'
4343

4444
// Mock file system operations.
4545
const mockReadJson = vi.hoisted(() => vi.fn())
46+
const mockSafeDelete = vi.hoisted(() => vi.fn())
4647
const mockSafeMkdir = vi.hoisted(() => vi.fn())
4748
const mockWriteJson = vi.hoisted(() => vi.fn())
4849

4950
// Mock fs promises.
5051
const mockFsWriteFile = vi.hoisted(() => vi.fn())
5152
const mockFsReadFile = vi.hoisted(() => vi.fn())
52-
const mockFsUnlink = vi.hoisted(() => vi.fn())
5353

5454
vi.mock('node:fs', async () => {
5555
const actual = await vi.importActual<typeof import('node:fs')>('node:fs')
@@ -60,13 +60,13 @@ vi.mock('node:fs', async () => {
6060
mkdir: vi.fn(),
6161
readFile: mockFsReadFile,
6262
writeFile: mockFsWriteFile,
63-
unlink: mockFsUnlink,
6463
},
6564
}
6665
})
6766

6867
vi.mock('@socketsecurity/lib/fs', () => ({
6968
readJson: mockReadJson,
69+
safeDelete: mockSafeDelete,
7070
safeMkdir: mockSafeMkdir,
7171
writeJson: mockWriteJson,
7272
}))
@@ -80,7 +80,7 @@ describe('ghsa-tracker', () => {
8080
// Default: lock file creation succeeds.
8181
mockFsWriteFile.mockResolvedValue(undefined)
8282
mockFsReadFile.mockResolvedValue('12345')
83-
mockFsUnlink.mockResolvedValue(undefined)
83+
mockSafeDelete.mockResolvedValue(undefined)
8484
})
8585

8686
describe('loadGhsaTracker', () => {
@@ -454,23 +454,8 @@ describe('ghsa-tracker', () => {
454454

455455
await markGhsaFixed(mockCwd, 'GHSA-release-lock', 123)
456456

457-
// Should attempt to unlink the lock file.
458-
expect(mockFsUnlink).toHaveBeenCalled()
459-
})
460-
461-
it('handles lock cleanup error gracefully', async () => {
462-
const existingTracker: GhsaTracker = {
463-
version: 1,
464-
fixed: [],
465-
}
466-
467-
mockReadJson.mockResolvedValue(existingTracker)
468-
mockFsUnlink.mockRejectedValueOnce(new Error('Cleanup error'))
469-
470-
// Should not throw.
471-
await expect(
472-
markGhsaFixed(mockCwd, 'GHSA-cleanup-error', 123),
473-
).resolves.toBeUndefined()
457+
// Should attempt to delete the lock file.
458+
expect(mockSafeDelete).toHaveBeenCalled()
474459
})
475460

476461
it('proceeds without lock when all attempts fail', async () => {

0 commit comments

Comments
 (0)