From bd3cc2f98c4cdfe13cf42ce4ad5f6f3d4d8e44fa Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:45:14 -0700 Subject: [PATCH] test(git): align admission lifetime contracts --- src/main/git/command-runner/git-spawn.ts | 12 +++++++---- .../filesystem-branch-compare-diff.test.ts | 2 +- ...lesystem-commit-message-generation.test.ts | 5 ++++- .../filesystem-git-commit-dispatch.test.ts | 4 +++- .../ipc/filesystem-git-status-staging.test.ts | 20 ++++++++++--------- src/main/ipc/filesystem-list-files.test.ts | 15 ++++++++++++++ 6 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/main/git/command-runner/git-spawn.ts b/src/main/git/command-runner/git-spawn.ts index 03ff6d24a98..47ccb501019 100644 --- a/src/main/git/command-runner/git-spawn.ts +++ b/src/main/git/command-runner/git-spawn.ts @@ -67,6 +67,8 @@ export async function withGitAdmission( return } finalized = true + child.off('error', handleError) + child.off('close', handleClose) grant?.release() if (error) { span.fail(error) @@ -74,21 +76,23 @@ export async function withGitAdmission( span.end() } } - child.on('error', (error) => { + const handleError = (error: Error): void => { if (!child.pid) { finalize(error) } else { liveError = error } - }) - child.once('close', (code, signal) => { + } + const handleClose = (code: number | null, signal: NodeJS.Signals | null): void => { const exitError = liveError ?? (code === 0 && signal === null ? undefined : new Error(`git exited with ${code ?? signal ?? 'unknown'}.`)) finalize(exitError) - }) + } + child.on('error', handleError) + child.once('close', handleClose) return child } catch (error) { grant?.release() diff --git a/src/main/ipc/filesystem-branch-compare-diff.test.ts b/src/main/ipc/filesystem-branch-compare-diff.test.ts index e7aff1b0e5e..53c4bcec4fc 100644 --- a/src/main/ipc/filesystem-branch-compare-diff.test.ts +++ b/src/main/ipc/filesystem-branch-compare-diff.test.ts @@ -242,7 +242,7 @@ describe('registerFilesystemHandlers', () => { filePath: path.join('src', 'file.ts'), oldPath: path.join('src', 'old-file.ts') }, - {} + { admissionTier: 'interactive' } ) }) diff --git a/src/main/ipc/filesystem-commit-message-generation.test.ts b/src/main/ipc/filesystem-commit-message-generation.test.ts index 591ea3565f9..8719c8d0f71 100644 --- a/src/main/ipc/filesystem-commit-message-generation.test.ts +++ b/src/main/ipc/filesystem-commit-message-generation.test.ts @@ -96,7 +96,9 @@ describe('registerFilesystemHandlers', () => { }) ).resolves.toEqual({ success: true, message: 'Update README' }) - expect(getStagedCommitContextMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, {}) + expect(getStagedCommitContextMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, { + admissionTier: 'interactive' + }) expect(generateCommitMessageFromContextMock).toHaveBeenCalledWith(context, params, { kind: 'local', cwd: WORKTREE_FEATURE_PATH @@ -253,6 +255,7 @@ describe('registerFilesystemHandlers', () => { }) expect(getStagedCommitContextMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, { + admissionTier: 'interactive', wslDistro: 'Ubuntu' }) expect(prepareForCodexLaunch).toHaveBeenCalledWith({ diff --git a/src/main/ipc/filesystem-git-commit-dispatch.test.ts b/src/main/ipc/filesystem-git-commit-dispatch.test.ts index 0f683936bd7..66b728b49a2 100644 --- a/src/main/ipc/filesystem-git-commit-dispatch.test.ts +++ b/src/main/ipc/filesystem-git-commit-dispatch.test.ts @@ -80,7 +80,9 @@ describe('registerFilesystemHandlers', () => { }) ).resolves.toEqual({ success: true }) - expect(commitChangesMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, 'feat: ship commit', {}) + expect(commitChangesMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, 'feat: ship commit', { + admissionTier: 'interactive' + }) }) it('returns local commit hook failure payload from git:commit', async () => { diff --git a/src/main/ipc/filesystem-git-status-staging.test.ts b/src/main/ipc/filesystem-git-status-staging.test.ts index 4f3a62ce679..a5f0b8a6216 100644 --- a/src/main/ipc/filesystem-git-status-staging.test.ts +++ b/src/main/ipc/filesystem-git-status-staging.test.ts @@ -94,11 +94,9 @@ describe('registerFilesystemHandlers', () => { // Why: validateGitRelativeFilePath uses path.relative() which produces // platform-specific separators (backslashes on Windows). - expect(stageFileMock).toHaveBeenCalledWith( - WORKTREE_FEATURE_PATH, - path.join('src', 'file.ts'), - {} - ) + expect(stageFileMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, path.join('src', 'file.ts'), { + admissionTier: 'interactive' + }) }) it('uses worktree roots seeded by worktrees:list without rebuilding the cache', async () => { @@ -346,7 +344,9 @@ describe('registerFilesystemHandlers', () => { connectionId: 'ssh-1' }) - expect(abortMergeMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, {}) + expect(abortMergeMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, { + admissionTier: 'interactive' + }) expect(sshProvider.abortMerge).toHaveBeenCalledWith('/remote/repo') }) @@ -366,7 +366,9 @@ describe('registerFilesystemHandlers', () => { connectionId: 'ssh-1' }) - expect(abortRebaseMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, {}) + expect(abortRebaseMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, { + admissionTier: 'interactive' + }) expect(sshProvider.abortRebase).toHaveBeenCalledWith('/remote/repo') }) @@ -410,7 +412,7 @@ describe('registerFilesystemHandlers', () => { expect(bulkStageFilesMock).toHaveBeenCalledWith( WORKTREE_FEATURE_PATH, [path.join('src', 'file.ts'), path.join('nested', 'child.ts')], - {} + { admissionTier: 'interactive' } ) }) @@ -427,7 +429,7 @@ describe('registerFilesystemHandlers', () => { expect(bulkDiscardChangesMock).toHaveBeenCalledWith( WORKTREE_FEATURE_PATH, [path.join('src', 'file.ts'), path.join('nested', 'child.ts')], - {} + { admissionTier: 'interactive' } ) }) diff --git a/src/main/ipc/filesystem-list-files.test.ts b/src/main/ipc/filesystem-list-files.test.ts index 4ad492ec10a..b1b06bd2d16 100644 --- a/src/main/ipc/filesystem-list-files.test.ts +++ b/src/main/ipc/filesystem-list-files.test.ts @@ -37,6 +37,7 @@ import { EventEmitter } from 'node:events' import type { Store } from '../persistence' import type { ChildProcess } from 'node:child_process' import { FileListingCancelledError } from '../../shared/file-listing-cancellation' +import { _gitAdmissionSnapshotForTests } from '../git/command-runner/git-subprocess-admission' const SHA1 = '0123456789abcdef0123456789abcdef01234567' @@ -603,6 +604,7 @@ describe('filesystem-list-files', () => { await expect(promise).resolves.toEqual(['src/kept.ts']) expect(primary.kill).toHaveBeenCalled() + primary.emit('close', null, 'SIGTERM') }) it('git fallback applies hidden dir blocklist', async () => { @@ -695,6 +697,15 @@ describe('filesystem-list-files', () => { expect(gitP2.kill).toHaveBeenCalled() expect((gitP1.stdout as unknown as EventEmitter).listenerCount('data')).toBe(0) expect((gitP1.stderr as unknown as EventEmitter).listenerCount('data')).toBe(0) + // Admission retains live children after the listing listeners detach. + const admittedBeforeClose = _gitAdmissionSnapshotForTests().budgets.general?.baseUsed + expect(gitP1.listenerCount('error')).toBe(1) + expect(gitP1.listenerCount('close')).toBe(1) + gitP1.emit('close', null, 'SIGKILL') + gitP2.emit('close', null, 'SIGKILL') + + expect(admittedBeforeClose).toBe(2) + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) expect(gitP1.listenerCount('error')).toBe(0) expect(gitP1.listenerCount('close')).toBe(0) } finally { @@ -748,6 +759,10 @@ describe('filesystem-list-files', () => { ) expect(gitP2.kill).toHaveBeenCalled() expect(warnSpy).toHaveBeenCalled() + const admittedBeforeClose = _gitAdmissionSnapshotForTests().budgets.general?.baseUsed + gitP2.emit('close', null, 'SIGKILL') + expect(admittedBeforeClose).toBe(1) + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed).toBe(0) } finally { vi.useRealTimers() warnSpy.mockRestore()