diff --git a/src/dsh/scan.ts b/src/dsh/scan.ts index abe90c8..e689995 100644 --- a/src/dsh/scan.ts +++ b/src/dsh/scan.ts @@ -17,7 +17,7 @@ import { getDshScannerMetadata } from './metadata.js'; import { inspectRegularFileWithinRoot } from '../scanner/safe-file.js'; import { walkDirectoryWithCoverage } from '../scanner/file-walker.js'; import { addFindingContext, calculateReviewPriority, runtimeSurfaceTags } from './finding-context.js'; -import { resolveDshSource } from './source.js'; +import { resolveDshSource, runWithDshCleanup } from './source.js'; import type { DshCapabilityProfile, DshFinding, @@ -157,7 +157,7 @@ export async function scanDshPlugin( options: ScanDshPluginOptions = {}, ): Promise { const source = await resolveDshSource(input, options); - try { + const { value: report, cleanupWarning } = await runWithDshCleanup(async (): Promise => { const directory = await walkDirectoryWithCoverage(source.rootDir, { includeGeneratedRuntime: true }); const detection = await detectDshPlugin(source.rootDir, directory.files); const capabilityProfile = await buildCapabilityProfile(source.rootDir, detection, directory.files); @@ -304,9 +304,9 @@ export async function scanDshPlugin( cordisParseErrors: detection.cordis.parseErrors, }, }; - } finally { - await source.cleanup(); - } + }, source.cleanup); + if (cleanupWarning) report.diagnostics.cleanupWarning = cleanupWarning; + return report; } export { DSH_RULES, RULES as DSH_SCAN_RULES }; diff --git a/src/dsh/source.ts b/src/dsh/source.ts index 43a659a..77faf3a 100644 --- a/src/dsh/source.ts +++ b/src/dsh/source.ts @@ -9,6 +9,20 @@ const GITHUB_REPO = /^https:\/\/github\.com\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+ export const MAX_GITHUB_ACQUISITION_BYTES = 256 * 1024 * 1024; export const MAX_GITHUB_OBJECTS = 100_000; const ACQUISITION_POLL_MS = 100; +const CLEANUP_MAX_RETRIES = 8; +const CLEANUP_RETRY_DELAY_MS = 100; + +export async function removeDshTempRoot( + tempRoot: string, + remove: typeof rm = rm, +): Promise { + await remove(tempRoot, { + recursive: true, + force: true, + maxRetries: CLEANUP_MAX_RETRIES, + retryDelay: CLEANUP_RETRY_DELAY_MS, + }); +} async function directoryBytesWithinBudget(rootDir: string, maxBytes: number): Promise { const pending = [rootDir]; @@ -46,30 +60,53 @@ export async function assertDshAcquisitionByteBudget( } } +interface KillableGitChild { + readonly killed: boolean; + kill(signal?: NodeJS.Signals | number): boolean; +} + +export function startDshAcquisitionMonitor( + child: KillableGitChild, + checkBudget: () => Promise, + pollMs = ACQUISITION_POLL_MS, +): { stop(): Promise } { + let budgetError: Error | undefined; + let activeCheck: Promise | undefined; + const monitor = setInterval(() => { + if (activeCheck || child.killed) return; + activeCheck = checkBudget().catch(error => { + budgetError = error as Error; + child.kill('SIGKILL'); + }).finally(() => { + activeCheck = undefined; + }); + }, pollMs); + return { + async stop(): Promise { + clearInterval(monitor); + await activeCheck; + return budgetError; + }, + }; +} + function execBoundedGit(args: string[], rootDir: string, timeout: number): Promise<{ stdout: string; stderr: string }> { return new Promise((resolvePromise, reject) => { - let budgetError: Error | undefined; - let checking = false; const child = execFile('git', args, { timeout, maxBuffer: 4 * 1024 * 1024, env: { ...process.env, GIT_TERMINAL_PROMPT: '0' }, }, (error, stdout, stderr) => { - clearInterval(monitor); - if (budgetError) reject(budgetError); - else if (error) reject(error); - else resolvePromise({ stdout, stderr }); + void monitor.stop().then(budgetError => { + if (budgetError) reject(budgetError); + else if (error) reject(error); + else resolvePromise({ stdout, stderr }); + }, reject); }); - const monitor = setInterval(() => { - if (checking || child.killed) return; - checking = true; - void assertDshAcquisitionByteBudget(rootDir).catch(error => { - budgetError = error as Error; - child.kill('SIGKILL'); - }).finally(() => { - checking = false; - }); - }, ACQUISITION_POLL_MS); + const monitor = startDshAcquisitionMonitor( + child, + () => assertDshAcquisitionByteBudget(rootDir), + ); }); } @@ -111,6 +148,45 @@ export interface ResolveDshSourceOptions { ref?: string; } +export interface DshCleanupResult { + value: T; + cleanupWarning?: string; +} + +function describeCleanupFailure(error: unknown): string { + const code = (error as NodeJS.ErrnoException).code; + return `Temporary GitHub checkout cleanup failed${code ? ` (${code})` : ''}; files may remain in the OS temporary directory`; +} + +export async function runWithDshCleanup( + operation: () => Promise, + cleanup: () => Promise, +): Promise> { + let value!: T; + let operationFailed = false; + let operationError: unknown; + try { + value = await operation(); + } catch (error) { + operationFailed = true; + operationError = error; + } + let cleanupWarning: string | undefined; + try { + await cleanup(); + } catch (error) { + cleanupWarning = describeCleanupFailure(error); + } + if (operationFailed) { + if (cleanupWarning) { + const message = operationError instanceof Error ? operationError.message : String(operationError); + throw new Error(`${message}; ${cleanupWarning}`, { cause: operationError }); + } + throw operationError; + } + return { value, cleanupWarning }; +} + async function gitMetadata(rootDir: string): Promise<{ revision?: string; lastCommitAt?: string }> { try { const [{ stdout: revision }, { stdout: lastCommitAt }] = await Promise.all([ @@ -240,11 +316,15 @@ export async function resolveDshSource( repositoryUrl, requestedRef, ...metadata, - cleanup: () => rm(tempRoot, { recursive: true, force: true }), + cleanup: () => removeDshTempRoot(tempRoot), }; } catch (error) { - await rm(tempRoot, { recursive: true, force: true }); - throw new Error(`Failed to fetch GitHub repository: ${(error as Error).message}`); + const { cleanupWarning } = await runWithDshCleanup( + async () => undefined, + () => removeDshTempRoot(tempRoot), + ); + const fetchMessage = `Failed to fetch GitHub repository: ${(error as Error).message}`; + throw new Error(cleanupWarning ? `${fetchMessage}; ${cleanupWarning}` : fetchMessage, { cause: error }); } } diff --git a/src/dsh/types.ts b/src/dsh/types.ts index 986e399..fe91b89 100644 --- a/src/dsh/types.ts +++ b/src/dsh/types.ts @@ -188,5 +188,7 @@ export interface DshPluginScanReport { diagnostics: { packageParseError?: string; cordisParseErrors: Array<{ file: string; message: string }>; + /** Operational warning only; does not affect the security risk result. */ + cleanupWarning?: string; }; } diff --git a/src/reports/dsh-report.ts b/src/reports/dsh-report.ts index 2016de9..e13ecd8 100644 --- a/src/reports/dsh-report.ts +++ b/src/reports/dsh-report.ts @@ -61,6 +61,9 @@ export function renderDshMarkdown(report: DshPluginScanReport): string { const runtimeSurfaceRisk = report.runtimeSurfaceRiskLevel ?? report.riskLevel; const runtimeSurfaceRecommendation = report.runtimeSurfaceRecommendation ?? report.installRecommendation; const scanCoverage = report.scanCoverage; + const cleanupWarning = report.diagnostics.cleanupWarning + ? `- Cleanup warning: ${markdownEscape(report.diagnostics.cleanupWarning)}\n` + : ''; const capabilities = Object.entries(report.capabilityProfile) .map(([key, enabled]) => `| ${CAPABILITY_LABELS[key as keyof typeof CAPABILITY_LABELS]} | ${enabled ? 'Yes' : 'No'} |`) .join('\n'); @@ -137,7 +140,7 @@ ${findings} - Artifact hash: ${report.identity.artifactHash ?? 'Unknown'} - Scanned at: ${report.scannedAt} - Files scanned: ${report.filesScanned} -- Scan coverage: ${scanCoverage +${cleanupWarning}- Scan coverage: ${scanCoverage ? `${scanCoverage.complete ? 'complete' : 'INCOMPLETE'} (${scanCoverage.scanned}/${scanCoverage.discovered}; skipped ${scanCoverage.skipped}: file limit ${scanCoverage.skippedByReason.fileLimit}, oversized ${scanCoverage.skippedByReason.oversized}, unreadable ${scanCoverage.skippedByReason.unreadable})` : 'Unavailable in legacy schema-v1 report'} @@ -155,6 +158,9 @@ export function renderDshHtml(report: DshPluginScanReport): string { const runtimeSurfaceRisk = report.runtimeSurfaceRiskLevel ?? report.riskLevel; const runtimeSurfaceRecommendation = report.runtimeSurfaceRecommendation ?? report.installRecommendation; const scanCoverage = report.scanCoverage; + const cleanupWarning = report.diagnostics.cleanupWarning + ? `
Cleanup warning
${htmlEscape(report.diagnostics.cleanupWarning)}
` + : ''; const scanCoverageText = scanCoverage ? `${scanCoverage.complete ? 'Complete' : 'INCOMPLETE'} — ${scanCoverage.scanned}/${scanCoverage.discovered} scanned; ${scanCoverage.skipped} skipped (file limit ${scanCoverage.skippedByReason.fileLimit}, oversized ${scanCoverage.skippedByReason.oversized}, unreadable ${scanCoverage.skippedByReason.unreadable})` : 'Unavailable in legacy schema-v1 report'; @@ -213,7 +219,7 @@ export function renderDshHtml(report: DshPluginScanReport): string {

Key findings

${findings}

Install recommendation

${htmlEscape(RECOMMENDATIONS[report.installRecommendation])}

Runtime surface: ${htmlEscape(RECOMMENDATIONS[runtimeSurfaceRecommendation])}

${report.harmlessMismatch ? '

Looks harmless, but requests elevated capabilities.

' : ''}
-

Artifact

Repository
${htmlEscape(report.project.repositoryUrl ?? 'Local directory')}
Requested ref
${htmlEscape(report.source.requestedRef ?? (report.source.kind === 'github' ? 'Default branch HEAD' : 'Not applicable'))}
Resolved revision
${htmlEscape(report.source.revision ?? 'Unknown')}
Last commit
${htmlEscape(report.source.lastCommitAt ?? 'Unknown')}
Files scanned
${report.filesScanned}
Scan coverage
${htmlEscape(scanCoverageText)}
Scanned
${htmlEscape(report.scannedAt)}
Hash
${htmlEscape(report.identity.artifactHash ?? 'Unknown')}
+

Artifact

Repository
${htmlEscape(report.project.repositoryUrl ?? 'Local directory')}
Requested ref
${htmlEscape(report.source.requestedRef ?? (report.source.kind === 'github' ? 'Default branch HEAD' : 'Not applicable'))}
Resolved revision
${htmlEscape(report.source.revision ?? 'Unknown')}
Last commit
${htmlEscape(report.source.lastCommitAt ?? 'Unknown')}
Files scanned
${report.filesScanned}
${cleanupWarning}
Scan coverage
${htmlEscape(scanCoverageText)}
Scanned
${htmlEscape(report.scannedAt)}
Hash
${htmlEscape(report.identity.artifactHash ?? 'Unknown')}
Static analysis can miss runtime-loaded behavior and cannot prove that a plugin is safe.
`; diff --git a/src/tests/dsh-source.test.ts b/src/tests/dsh-source.test.ts new file mode 100644 index 0000000..7e7778e --- /dev/null +++ b/src/tests/dsh-source.test.ts @@ -0,0 +1,105 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { access, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + removeDshTempRoot, + runWithDshCleanup, + startDshAcquisitionMonitor, +} from '../dsh/source.js'; +import { scanDshPlugin } from '../dsh/scan.js'; +import { renderDshHtml, renderDshMarkdown } from '../reports/dsh-report.js'; + +function busyError(): NodeJS.ErrnoException { + const error = new Error('resource busy or locked') as NodeJS.ErrnoException; + error.code = 'EBUSY'; + return error; +} + +describe('DSH GitHub source cleanup', () => { + it('enables retries when requesting recursive temporary directory removal', async () => { + let receivedOptions: Parameters[1]; + await removeDshTempRoot('temporary-checkout', async (_path, options) => { + receivedOptions = options; + }); + + assert.equal(receivedOptions!.recursive, true); + assert.equal(receivedOptions!.force, true); + assert.ok((receivedOptions!.maxRetries ?? 0) > 0); + assert.ok((receivedOptions!.retryDelay ?? 0) > 0); + }); + + it('waits for an in-flight acquisition budget check before stopping', async () => { + let markStarted!: () => void; + const started = new Promise(resolve => { markStarted = resolve; }); + let releaseCheck!: () => void; + const release = new Promise(resolve => { releaseCheck = resolve; }); + const child = { killed: false, kill: () => true }; + const monitor = startDshAcquisitionMonitor(child, async () => { + markStarted(); + await release; + }, 1); + await started; + + let stopped = false; + const stopping = monitor.stop().then(error => { + stopped = true; + return error; + }); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(stopped, false); + + releaseCheck(); + assert.equal(await stopping, undefined); + assert.equal(stopped, true); + }); + + it('removes a temporary GitHub checkout tree', async () => { + const tempRoot = await mkdtemp(join(tmpdir(), 'agentguard-dsh-cleanup-test-')); + await writeFile(join(tempRoot, 'pack-file.pack'), 'pack'); + + await removeDshTempRoot(tempRoot); + + await assert.rejects(() => access(tempRoot), { code: 'ENOENT' }); + }); + + it('returns a completed operation when temporary cleanup remains busy', async () => { + const result = await runWithDshCleanup( + async () => ({ report: 'complete' }), + async () => { throw busyError(); }, + ); + + assert.deepEqual(result.value, { report: 'complete' }); + assert.match(result.cleanupWarning ?? '', /EBUSY/); + }); + + it('preserves the operation failure when temporary cleanup also fails', async () => { + await assert.rejects( + () => runWithDshCleanup( + async () => { throw new Error('scan failed'); }, + async () => { throw busyError(); }, + ), + error => { + assert.match((error as Error).message, /^scan failed/); + assert.match((error as Error).message, /cleanup failed \(EBUSY\)/); + return true; + }, + ); + }); + + it('renders a non-fatal temporary cleanup warning in scan reports', async () => { + const root = await mkdtemp(join(tmpdir(), 'agentguard-dsh-cleanup-report-test-')); + try { + await writeFile(join(root, 'package.json'), JSON.stringify({ name: 'cleanup-report-test' })); + const report = await scanDshPlugin(root); + report.diagnostics.cleanupWarning = 'Temporary GitHub checkout cleanup failed (EBUSY)'; + + assert.match(renderDshMarkdown(report), /Cleanup warning:.*EBUSY/); + assert.match(renderDshHtml(report), /Cleanup warning.*EBUSY/); + assert.equal(report.riskLevel, 'low'); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); +});