From ee1211e0bfad063bdd61dc278ba37aebf694fd25 Mon Sep 17 00:00:00 2001 From: Adam Creeger Date: Thu, 21 May 2026 18:42:02 -0400 Subject: [PATCH] fix(finish): allow finish for draft-PR looms with closed issues and merged PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow `il finish` to proceed when the issue is already closed if the loom has an associated draft PR. Add early exit path when the draft PR is already merged/closed — skip rebase/validate/commit and go straight to cleanup. Fix ignite test mock to include SettingsManager dependency. --- src/commands/finish.test.ts | 138 ++++++++++++++++++++++++++++++++++++ src/commands/finish.ts | 63 ++++++++++++++-- src/commands/ignite.test.ts | 12 +++- 3 files changed, 205 insertions(+), 8 deletions(-) diff --git a/src/commands/finish.test.ts b/src/commands/finish.test.ts index 9526a458..340350d5 100644 --- a/src/commands/finish.test.ts +++ b/src/commands/finish.test.ts @@ -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({ diff --git a/src/commands/finish.ts b/src/commands/finish.ts index fd5d8abe..56536ae1 100644 --- a/src/commands/finish.ts +++ b/src/commands/finish.ts @@ -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': { @@ -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)') diff --git a/src/commands/ignite.test.ts b/src/commands/ignite.test.ts index 7183093c..13483ce3 100644 --- a/src/commands/ignite.test.ts +++ b/src/commands/ignite.test.ts @@ -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, ) })