Skip to content

Commit 089dabe

Browse files
acreegeracreeger
committed
Implement detection and committing of uncommitted changes when running 'finish' command
Fixes #49
1 parent fb51d63 commit 089dabe

File tree

5 files changed

+846
-10
lines changed

5 files changed

+846
-10
lines changed

‎src/commands/finish.test.ts‎

Lines changed: 155 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,15 @@ import { FinishCommand } from './finish.js'
33
import { GitHubService } from '../lib/GitHubService.js'
44
import { GitWorktreeManager } from '../lib/GitWorktreeManager.js'
55
import { ValidationRunner } from '../lib/ValidationRunner.js'
6+
import { CommitManager } from '../lib/CommitManager.js'
67
import type { Issue, PullRequest } from '../types/index.js'
78
import type { GitWorktree } from '../types/worktree.js'
89

910
// Mock dependencies
1011
vi.mock('../lib/GitHubService.js')
1112
vi.mock('../lib/GitWorktreeManager.js')
1213
vi.mock('../lib/ValidationRunner.js')
14+
vi.mock('../lib/CommitManager.js')
1315

1416
// Mock the logger to prevent console output during tests
1517
vi.mock('../utils/logger.js', () => ({
@@ -27,11 +29,13 @@ describe('FinishCommand', () => {
2729
let mockGitHubService: GitHubService
2830
let mockGitWorktreeManager: GitWorktreeManager
2931
let mockValidationRunner: ValidationRunner
32+
let mockCommitManager: CommitManager
3033

3134
beforeEach(() => {
3235
mockGitHubService = new GitHubService()
3336
mockGitWorktreeManager = new GitWorktreeManager()
3437
mockValidationRunner = new ValidationRunner()
38+
mockCommitManager = new CommitManager()
3539

3640
// Mock ValidationRunner.runValidations to always succeed by default
3741
vi.mocked(mockValidationRunner.runValidations).mockResolvedValue({
@@ -40,10 +44,24 @@ describe('FinishCommand', () => {
4044
totalDuration: 0,
4145
})
4246

47+
// Mock CommitManager.detectUncommittedChanges to return no changes by default
48+
vi.mocked(mockCommitManager.detectUncommittedChanges).mockResolvedValue({
49+
hasUncommittedChanges: false,
50+
unstagedFiles: [],
51+
stagedFiles: [],
52+
currentBranch: 'main',
53+
isAheadOfRemote: false,
54+
isBehindRemote: false,
55+
})
56+
57+
// Mock CommitManager.commitChanges to succeed by default
58+
vi.mocked(mockCommitManager.commitChanges).mockResolvedValue(undefined)
59+
4360
command = new FinishCommand(
4461
mockGitHubService,
4562
mockGitWorktreeManager,
46-
mockValidationRunner
63+
mockValidationRunner,
64+
mockCommitManager
4765
)
4866
})
4967

@@ -1277,6 +1295,136 @@ describe('FinishCommand', () => {
12771295
})
12781296
})
12791297

1298+
describe('workflow execution order', () => {
1299+
it('should run validation BEFORE detecting and committing changes', async () => {
1300+
// Test the correct workflow order: validate → detect → commit
1301+
const executionOrder: string[] = []
1302+
1303+
// Mock ValidationRunner to track execution order
1304+
vi.mocked(mockValidationRunner.runValidations).mockImplementation(async () => {
1305+
executionOrder.push('validation')
1306+
return {
1307+
success: true,
1308+
steps: [],
1309+
totalDuration: 0,
1310+
}
1311+
})
1312+
1313+
// Mock CommitManager to track execution order
1314+
vi.mocked(mockCommitManager.detectUncommittedChanges).mockImplementation(async () => {
1315+
executionOrder.push('commit-detect')
1316+
return {
1317+
hasUncommittedChanges: true,
1318+
unstagedFiles: ['src/test.ts'],
1319+
stagedFiles: [],
1320+
currentBranch: 'feat/issue-123',
1321+
isAheadOfRemote: false,
1322+
isBehindRemote: false,
1323+
}
1324+
})
1325+
1326+
vi.mocked(mockCommitManager.commitChanges).mockImplementation(async () => {
1327+
executionOrder.push('commit-execute')
1328+
})
1329+
1330+
vi.mocked(mockGitHubService.detectInputType).mockResolvedValue({
1331+
type: 'issue',
1332+
number: 123,
1333+
rawInput: '123',
1334+
})
1335+
vi.mocked(mockGitHubService.fetchIssue).mockResolvedValue({
1336+
number: 123,
1337+
title: 'Test Issue',
1338+
state: 'open',
1339+
body: '',
1340+
labels: [],
1341+
assignees: [],
1342+
url: 'https://github.com/test/repo/issues/123',
1343+
} as Issue)
1344+
vi.mocked(mockGitWorktreeManager.findWorktreesByIdentifier).mockResolvedValue([
1345+
{ path: '/test/issue-123', branch: 'feat/issue-123', commit: 'abc123', bare: false },
1346+
] as GitWorktree[])
1347+
1348+
// This should succeed with the correct order
1349+
await expect(
1350+
command.execute({
1351+
identifier: '123',
1352+
options: {},
1353+
})
1354+
).resolves.not.toThrow()
1355+
1356+
// ✅ CORRECT: The implementation should follow this order
1357+
expect(executionOrder).toEqual([
1358+
'validation', // ✅ First: Ensure code quality
1359+
'commit-detect', // ✅ Second: Check if there are changes to commit
1360+
'commit-execute' // ✅ Third: Only commit if validation passed
1361+
])
1362+
})
1363+
1364+
it('should NOT commit if validation fails', async () => {
1365+
// Test that validation failure prevents committing
1366+
const executionOrder: string[] = []
1367+
1368+
// Mock ValidationRunner to simulate failure
1369+
vi.mocked(mockValidationRunner.runValidations).mockImplementation(async () => {
1370+
executionOrder.push('validation')
1371+
throw new Error('Validation failed: TypeScript errors found')
1372+
})
1373+
1374+
// Mock CommitManager - these should NOT be called if validation fails
1375+
vi.mocked(mockCommitManager.detectUncommittedChanges).mockImplementation(async () => {
1376+
executionOrder.push('commit-detect')
1377+
return {
1378+
hasUncommittedChanges: true,
1379+
unstagedFiles: ['src/test.ts'],
1380+
stagedFiles: [],
1381+
currentBranch: 'feat/issue-123',
1382+
isAheadOfRemote: false,
1383+
isBehindRemote: false,
1384+
}
1385+
})
1386+
1387+
vi.mocked(mockCommitManager.commitChanges).mockImplementation(async () => {
1388+
executionOrder.push('commit-execute')
1389+
})
1390+
1391+
vi.mocked(mockGitHubService.detectInputType).mockResolvedValue({
1392+
type: 'issue',
1393+
number: 123,
1394+
rawInput: '123',
1395+
})
1396+
vi.mocked(mockGitHubService.fetchIssue).mockResolvedValue({
1397+
number: 123,
1398+
title: 'Test Issue',
1399+
state: 'open',
1400+
body: '',
1401+
labels: [],
1402+
assignees: [],
1403+
url: 'https://github.com/test/repo/issues/123',
1404+
} as Issue)
1405+
vi.mocked(mockGitWorktreeManager.findWorktreesByIdentifier).mockResolvedValue([
1406+
{ path: '/test/issue-123', branch: 'feat/issue-123', commit: 'abc123', bare: false },
1407+
] as GitWorktree[])
1408+
1409+
// This should fail at validation step
1410+
await expect(
1411+
command.execute({
1412+
identifier: '123',
1413+
options: {},
1414+
})
1415+
).rejects.toThrow('Validation failed: TypeScript errors found')
1416+
1417+
// ✅ CORRECT: Validation fails, so we never detect or commit changes
1418+
expect(executionOrder).toEqual([
1419+
'validation' // ✅ Validation fails, workflow stops here
1420+
])
1421+
1422+
// Verify CommitManager methods were never called
1423+
expect(mockCommitManager.detectUncommittedChanges).not.toHaveBeenCalled()
1424+
expect(mockCommitManager.commitChanges).not.toHaveBeenCalled()
1425+
})
1426+
})
1427+
12801428
describe('dependency injection', () => {
12811429
it('should accept GitHubService via constructor', () => {
12821430
const customService = new GitHubService()
@@ -1296,6 +1444,12 @@ describe('FinishCommand', () => {
12961444
expect(cmd).toBeDefined()
12971445
})
12981446

1447+
it('should accept CommitManager via constructor', () => {
1448+
const customCommitManager = new CommitManager()
1449+
const cmd = new FinishCommand(undefined, undefined, undefined, customCommitManager)
1450+
expect(cmd).toBeDefined()
1451+
})
1452+
12991453
it('should create default instances when not provided', () => {
13001454
const cmd = new FinishCommand()
13011455
expect(cmd).toBeDefined()

‎src/commands/finish.ts‎

Lines changed: 41 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import { logger } from '../utils/logger.js'
22
import { GitHubService } from '../lib/GitHubService.js'
33
import { GitWorktreeManager } from '../lib/GitWorktreeManager.js'
44
import { ValidationRunner } from '../lib/ValidationRunner.js'
5-
import type { FinishOptions, GitWorktree } from '../types/index.js'
5+
import { CommitManager } from '../lib/CommitManager.js'
6+
import type { FinishOptions, GitWorktree, CommitOptions } from '../types/index.js'
67
import path from 'path'
78

89
export interface FinishCommandInput {
@@ -22,16 +23,19 @@ export class FinishCommand {
2223
private gitHubService: GitHubService
2324
private gitWorktreeManager: GitWorktreeManager
2425
private validationRunner: ValidationRunner
26+
private commitManager: CommitManager
2527

2628
constructor(
2729
gitHubService?: GitHubService,
2830
gitWorktreeManager?: GitWorktreeManager,
29-
validationRunner?: ValidationRunner
31+
validationRunner?: ValidationRunner,
32+
commitManager?: CommitManager
3033
) {
3134
// Dependency injection for testing
3235
this.gitHubService = gitHubService ?? new GitHubService()
3336
this.gitWorktreeManager = gitWorktreeManager ?? new GitWorktreeManager()
3437
this.validationRunner = validationRunner ?? new ValidationRunner()
38+
this.commitManager = commitManager ?? new CommitManager()
3539
}
3640

3741
/**
@@ -48,13 +52,14 @@ export class FinishCommand {
4852
// Step 3: Log success
4953
logger.info(`Validated input: ${this.formatParsedInput(parsed)}`)
5054

51-
// Step 4: Run pre-merge validations (Sub-Issue #47)
52-
if (!input.options.dryRun) {
53-
const worktree = worktrees[0]
54-
if (!worktree) {
55-
throw new Error('No worktree found')
56-
}
55+
// Get worktree for steps 4 and 5
56+
const worktree = worktrees[0]
57+
if (!worktree) {
58+
throw new Error('No worktree found')
59+
}
5760

61+
// Step 4: Run pre-merge validations FIRST (Sub-Issue #47)
62+
if (!input.options.dryRun) {
5863
logger.info('Running pre-merge validations...')
5964

6065
await this.validationRunner.runValidations(worktree.path, {
@@ -65,7 +70,34 @@ export class FinishCommand {
6570
logger.info('[DRY RUN] Would run pre-merge validations')
6671
}
6772

68-
// Step 5: Additional workflow steps (PLACEHOLDER - implemented in later sub-issues)
73+
// Step 5: Detect uncommitted changes AFTER validation passes
74+
const gitStatus = await this.commitManager.detectUncommittedChanges(worktree.path)
75+
76+
// Step 6: Commit changes only if validation passed AND changes exist
77+
if (gitStatus.hasUncommittedChanges) {
78+
if (input.options.dryRun) {
79+
logger.info('[DRY RUN] Would auto-commit uncommitted changes (validation passed)')
80+
} else {
81+
logger.info('Validation passed, auto-committing uncommitted changes...')
82+
83+
const commitOptions: CommitOptions = {
84+
dryRun: input.options.dryRun ?? false,
85+
}
86+
87+
// Only add issueNumber if it's an issue
88+
if (parsed.type === 'issue' && parsed.number) {
89+
commitOptions.issueNumber = parsed.number
90+
}
91+
92+
await this.commitManager.commitChanges(worktree.path, commitOptions)
93+
94+
logger.success('Changes committed successfully')
95+
}
96+
} else {
97+
logger.debug('No uncommitted changes found')
98+
}
99+
100+
// Step 6: Additional workflow steps (PLACEHOLDER - implemented in later sub-issues)
69101
// Migration handling, rebasing, merging, etc.
70102
if (input.options.dryRun) {
71103
logger.info('[DRY RUN] Would proceed with finish workflow')

0 commit comments

Comments
 (0)