Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 138 additions & 0 deletions src/commands/finish.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1676,6 +1676,144 @@ describe('FinishCommand', () => {
expect(mockValidationRunner.runValidations).toHaveBeenCalled()
})

it('should allow closed issue for draft-PR loom (skip to cleanup)', async () => {
vi.mocked(mockIdentifierParser.parseForPatternDetection).mockResolvedValue({
type: 'issue',
number: 123,
originalInput: '123',
})

const mockIssue: Issue = {
number: 123,
title: 'Test Issue',
body: 'Test description',
state: 'closed',
labels: [],
assignees: [],
url: 'https://github.com/test/repo/issues/123',
}

vi.mocked(mockGitHubService.fetchIssue).mockResolvedValue(mockIssue)
vi.mocked(mockGitWorktreeManager.findWorktreeForIssue).mockResolvedValue(mockWorktree)

// Mock MetadataManager to return draft PR metadata
const { MetadataManager } = await import('../lib/MetadataManager.js')
vi.spyOn(MetadataManager.prototype, 'readMetadata').mockResolvedValue({
draftPrNumber: 55,
prUrls: { '55': 'https://github.com/test/repo/pull/55' },
} as never)

// Mock settings for draft-pr mode
vi.spyOn(SettingsManager.prototype, 'loadSettings').mockResolvedValue({
mainBranch: 'main',
worktreeDir: '/test/worktrees',
mergeBehavior: { mode: 'draft-pr' },
})

// Mock fetchPR to return a merged PR
vi.mocked(mockGitHubService.fetchPR).mockResolvedValue({
number: 55,
title: 'Draft PR',
state: 'merged',
branch: 'feat/issue-123__test',
url: 'https://github.com/test/repo/pull/55',
})

await command.execute({
identifier: '123',
options: {},
})

// Should NOT throw - draft-PR loom with closed issue is allowed
expect(mockGitHubService.fetchIssue).toHaveBeenCalledWith(123, undefined)
// Should skip rebase/validate/commit and go straight to cleanup
expect(mockMergeManager.rebaseOnMain).not.toHaveBeenCalled()
expect(mockValidationRunner.runValidations).not.toHaveBeenCalled()
})

it('should still throw for closed issue when loom has no draft PR', async () => {
vi.mocked(mockIdentifierParser.parseForPatternDetection).mockResolvedValue({
type: 'issue',
number: 123,
originalInput: '123',
})

const mockIssue: Issue = {
number: 123,
title: 'Test Issue',
body: 'Test description',
state: 'closed',
labels: [],
assignees: [],
url: 'https://github.com/test/repo/issues/123',
}

vi.mocked(mockGitHubService.fetchIssue).mockResolvedValue(mockIssue)
vi.mocked(mockGitWorktreeManager.findWorktreeForIssue).mockResolvedValue(mockWorktree)

// MetadataManager returns null (no draft PR)
const { MetadataManager } = await import('../lib/MetadataManager.js')
vi.spyOn(MetadataManager.prototype, 'readMetadata').mockResolvedValue(null)

await expect(
command.execute({
identifier: '123',
options: {},
})
).rejects.toThrow('Issue #123 is closed. Use --force to finish anyway.')
})

it('should respect --no-cleanup for merged draft PR with closed issue', async () => {
vi.mocked(mockIdentifierParser.parseForPatternDetection).mockResolvedValue({
type: 'issue',
number: 123,
originalInput: '123',
})

const mockIssue: Issue = {
number: 123,
title: 'Test Issue',
body: 'Test description',
state: 'closed',
labels: [],
assignees: [],
url: 'https://github.com/test/repo/issues/123',
}

vi.mocked(mockGitHubService.fetchIssue).mockResolvedValue(mockIssue)
vi.mocked(mockGitWorktreeManager.findWorktreeForIssue).mockResolvedValue(mockWorktree)

const { MetadataManager } = await import('../lib/MetadataManager.js')
vi.spyOn(MetadataManager.prototype, 'readMetadata').mockResolvedValue({
draftPrNumber: 55,
prUrls: { '55': 'https://github.com/test/repo/pull/55' },
} as never)

vi.spyOn(SettingsManager.prototype, 'loadSettings').mockResolvedValue({
mainBranch: 'main',
worktreeDir: '/test/worktrees',
mergeBehavior: { mode: 'draft-pr' },
})

vi.mocked(mockGitHubService.fetchPR).mockResolvedValue({
number: 55,
title: 'Draft PR',
state: 'merged',
branch: 'feat/issue-123__test',
url: 'https://github.com/test/repo/pull/55',
})

await command.execute({
identifier: '123',
options: { cleanup: false },
})

// Should not perform cleanup
expect(mockResourceCleanup.cleanupWorktree).not.toHaveBeenCalled()
// Should not attempt rebase/validate/commit
expect(mockMergeManager.rebaseOnMain).not.toHaveBeenCalled()
})

it('should throw error if issue not found on GitHub', async () => {
// Mock parseForPatternDetection to return issue type
vi.mocked(mockIdentifierParser.parseForPatternDetection).mockResolvedValue({
Expand Down
63 changes: 57 additions & 6 deletions src/commands/finish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -535,17 +535,29 @@ export class FinishCommand {
// Fetch issue from GitHub
const issue = await this.issueTracker.fetchIssue(parsed.number, repo)

// Validate issue state (warn if closed unless --force)
// Find associated worktree (needed before closed-issue check to read metadata)
const issueWorktrees = await this.findWorktreeForIdentifier(parsed)

// Validate issue state (warn if closed unless --force or draft-PR loom)
if (issue.state === 'closed' && !options.force) {
throw new Error(
`Issue #${parsed.number} is closed. Use --force to finish anyway.`
)
const worktree = issueWorktrees[0]
let isDraftPrLoom = false
if (worktree) {
const metadataManager = new MetadataManager()
const metadata = await metadataManager.readMetadata(worktree.path)
isDraftPrLoom = !!metadata?.draftPrNumber
}
if (!isDraftPrLoom) {
throw new Error(
`Issue #${parsed.number} is closed. Use --force to finish anyway.`
)
}
getLogger().debug(`Issue #${parsed.number} is closed but loom has a draft PR - allowing finish for cleanup`)
}

getLogger().debug(`Validated issue #${parsed.number} (state: ${issue.state})`)

// Find associated worktree
return await this.findWorktreeForIdentifier(parsed)
return issueWorktrees
}

case 'branch': {
Expand Down Expand Up @@ -683,6 +695,45 @@ export class FinishCommand {
jsonStream: options.jsonStream ?? false,
}

// Early exit: if this is a draft-PR loom whose PR is already merged/closed,
// skip all rebase/validate/commit steps and go straight to cleanup
const earlySettings = await this.settingsManager.loadSettings(worktree.path)
const earlyMergeBehavior = earlySettings.mergeBehavior ?? { mode: 'local' }
const earlyRawMode = earlyMergeBehavior.mode as string
const earlyMergeMode = earlyRawMode === 'github-draft-pr' ? 'draft-pr' : earlyRawMode
if (earlyMergeMode === 'draft-pr') {
const earlyMetadataManager = new MetadataManager()
const earlyMetadata = await earlyMetadataManager.readMetadata(worktree.path)
if (earlyMetadata?.draftPrNumber) {
try {
const draftPr = await this.fetchPullRequest(earlyMetadata.draftPrNumber)
if (draftPr.state === 'merged' || draftPr.state === 'closed') {
getLogger().info(`Draft PR #${earlyMetadata.draftPrNumber} is already ${draftPr.state.toUpperCase()} - skipping to cleanup`)

if (!options.dryRun) {
await earlyMetadataManager.archiveMetadata(worktree.path)
}

if (options.cleanup === false) {
getLogger().info('Worktree kept active (--no-cleanup flag)')
getLogger().info(`To cleanup later: il cleanup ${parsed.originalInput}`)
} else {
await this.performPRCleanup(parsed, options, worktree, draftPr.state as 'closed' | 'merged', result)
getLogger().success(`Draft PR #${earlyMetadata.draftPrNumber} cleanup completed`)
result.operations.push({
type: 'cleanup',
message: `Draft PR #${earlyMetadata.draftPrNumber} cleanup completed`,
success: true,
})
}
return
}
} catch (error) {
getLogger().debug(`Failed to fetch draft PR #${earlyMetadata.draftPrNumber} state: ${error instanceof Error ? error.message : String(error)}`)
}
}
}

// Skip rebase/validation/commit steps if --skip-to-pr flag is set (debug mode)
if (options.skipToPr) {
getLogger().info('Skipping rebase/validation/commit (--skip-to-pr flag)')
Expand Down
12 changes: 10 additions & 2 deletions src/commands/ignite.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,18 @@ describe('IgniteCommand', () => {
}),
} as unknown as GitWorktreeManager

// Create command with mocked dependencies
const mockSettingsManager = {
loadSettings: vi.fn().mockResolvedValue({}),
getSpinModel: vi.fn().mockReturnValue('opus'),
getSpinEffort: vi.fn().mockReturnValue('high'),
}

// Create command with mocked dependencies (including SettingsManager to avoid reading real config)
command = new IgniteCommand(
mockTemplateManager,
mockGitWorktreeManager
mockGitWorktreeManager,
undefined,
mockSettingsManager as never,
)
})

Expand Down
Loading