diff --git a/.changeset/feedback-missing-label-retry.md b/.changeset/feedback-missing-label-retry.md new file mode 100644 index 00000000..14354c30 --- /dev/null +++ b/.changeset/feedback-missing-label-retry.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Fix `openspec feedback` failing when the repository does not define the `feedback` label. The command now retries without the label and notes that it was not applied, instead of exiting with an error and discarding the feedback. diff --git a/openspec/specs/cli-feedback/spec.md b/openspec/specs/cli-feedback/spec.md index 188142b1..b3a4b022 100644 --- a/openspec/specs/cli-feedback/spec.md +++ b/openspec/specs/cli-feedback/spec.md @@ -16,6 +16,15 @@ The system SHALL provide an `openspec feedback` command that creates a GitHub Is - **AND** the issue has the `feedback` label - **AND** the system displays the created issue URL +#### Scenario: Repository does not define the feedback label + +- **WHEN** user executes `openspec feedback "Great tool!"` +- **AND** the repository does not define the `feedback` label, so `gh` refuses to create the issue +- **THEN** the system retries `gh issue create` without the label +- **AND** the issue is created in the openspec repository without the `feedback` label +- **AND** the system displays the created issue URL +- **AND** the system notes that the label was not applied + #### Scenario: Safe command execution - **WHEN** submitting feedback via `gh` CLI @@ -127,9 +136,10 @@ The system SHALL handle feedback submission errors gracefully. #### Scenario: gh CLI execution failure -- **WHEN** `gh issue create` command fails +- **WHEN** `gh issue create` command fails for any reason other than the repository not defining the `feedback` label - **THEN** the system displays the error output from `gh` CLI - **AND** exits with the same exit code as `gh` +- **AND** does not retry the submission #### Scenario: Network failure diff --git a/src/commands/feedback.ts b/src/commands/feedback.ts index e157d11e..52926040 100644 --- a/src/commands/feedback.ts +++ b/src/commands/feedback.ts @@ -119,41 +119,90 @@ function displayFormattedFeedback(title: string, body: string): void { } /** - * Submit feedback via gh CLI + * Check whether gh refused the issue because the repository does not define + * the label. gh resolves label names before creating the issue, so this + * failure means no issue was created. + * + * Only gh's stderr is inspected. The error message also embeds the command + * line, which carries the user's own feedback text. + */ +function isMissingLabelError(error: any): boolean { + return /could not add label/i.test(error?.stderr?.toString() ?? ''); +} + +/** + * Report a gh CLI failure and exit, preserving gh's exit code + */ +function reportGhFailure(error: any): void { + // Display the error output from gh CLI + if (error.stderr) { + console.error(error.stderr.toString()); + } else if (error.message) { + console.error(error.message); + } + + // Exit with the same code as gh CLI + process.exit(error.status ?? 1); +} + +/** + * Create the feedback issue via gh CLI * Uses execFileSync to prevent shell injection vulnerabilities */ -function submitViaGhCli(title: string, body: string): void { - try { - const result = execFileSync( - 'gh', - [ - 'issue', - 'create', - '--repo', - 'Fission-AI/OpenSpec', - '--title', - title, - '--body', - body, - '--label', - 'feedback', - ], - { encoding: 'utf-8', stdio: 'pipe' } - ); +function createIssue(title: string, body: string, labels: string[]): string { + const args = [ + 'issue', + 'create', + '--repo', + 'Fission-AI/OpenSpec', + '--title', + title, + '--body', + body, + ]; - const issueUrl = result.trim(); - console.log(`\nāœ“ Feedback submitted successfully!`); - console.log(`Issue URL: ${issueUrl}\n`); + for (const label of labels) { + args.push('--label', label); + } + + const result = execFileSync('gh', args, { encoding: 'utf-8', stdio: 'pipe' }); + + return result.trim(); +} + +/** + * Submit feedback via gh CLI + */ +function submitViaGhCli(title: string, body: string): void { + let issueUrl: string; + let labelApplied = true; + + try { + issueUrl = createIssue(title, body, ['feedback']); } catch (error: any) { - // Display the error output from gh CLI - if (error.stderr) { - console.error(error.stderr.toString()); - } else if (error.message) { - console.error(error.message); + if (!isMissingLabelError(error)) { + reportGhFailure(error); + return; } - // Exit with the same code as gh CLI - process.exit(error.status ?? 1); + // The repository does not define the 'feedback' label. Nothing was + // created, so retry unlabeled rather than dropping the feedback. + try { + issueUrl = createIssue(title, body, []); + labelApplied = false; + } catch (retryError: any) { + reportGhFailure(retryError); + return; + } + } + + console.log(`\nāœ“ Feedback submitted successfully!`); + console.log(`Issue URL: ${issueUrl}\n`); + + if (!labelApplied) { + console.log( + "Note: created without the 'feedback' label because the repository does not define it.\n" + ); } } diff --git a/test/commands/feedback.test.ts b/test/commands/feedback.test.ts index 7a2125f1..e5925718 100644 --- a/test/commands/feedback.test.ts +++ b/test/commands/feedback.test.ts @@ -198,6 +198,12 @@ describe('FeedbackCommand', () => { expect(consoleLogSpy).toHaveBeenCalledWith( expect.stringContaining(issueUrl) ); + + // Only one attempt, and no note about a dropped label + expect(mockExecFileSync).toHaveBeenCalledTimes(1); + expect(consoleLogSpy).not.toHaveBeenCalledWith( + expect.stringContaining("without the 'feedback' label") + ); }); it('should include --body flag when body is provided', async () => { @@ -327,17 +333,147 @@ describe('FeedbackCommand', () => { throw error; }); - try { - await feedbackCommand.execute('Test'); - } catch (error: any) { - // Should exit with the same code as gh CLI - expect(error.message).toBe('process.exit(1)'); - } + await expect(feedbackCommand.execute('Test')).rejects.toThrow( + 'process.exit(1)' + ); // Should display the error from gh CLI expect(consoleErrorSpy).toHaveBeenCalledWith( expect.stringContaining('Network connectivity issue') ); + + // A non-label failure must NOT be retried + expect(mockExecFileSync).toHaveBeenCalledTimes(1); + }); + + it('should not retry when the feedback text mentions the label error', async () => { + mockExecSync.mockImplementation((cmd: string, options?: any) => { + if (cmd === 'which gh' || cmd === 'where gh') { + return Buffer.from('/usr/local/bin/gh'); + } + if (cmd === 'gh auth status') { + return Buffer.from('Logged in'); + } + return ''; + }); + + // gh fails for an unrelated reason. Node puts the whole command line ā€” + // including the user's own words ā€” into error.message, so only stderr + // may decide whether this was a label failure. + mockExecFileSync.mockImplementation((_cmd: string, args: string[]) => { + const error: any = new Error( + `Command failed: gh ${args.join(' ')}\nerror connecting to api.github.com` + ); + error.status = 1; + error.stderr = Buffer.from('error connecting to api.github.com'); + throw error; + }); + + await expect( + feedbackCommand.execute('gh could not add label bug report') + ).rejects.toThrow('process.exit(1)'); + + expect(mockExecFileSync).toHaveBeenCalledTimes(1); + expect(consoleLogSpy).not.toHaveBeenCalledWith( + expect.stringContaining("without the 'feedback' label") + ); + }); + + it('should retry without the label when the repo does not define it', async () => { + const issueUrl = 'https://github.com/Fission-AI/OpenSpec/issues/129'; + + mockExecSync.mockImplementation((cmd: string, options?: any) => { + if (cmd === 'which gh' || cmd === 'where gh') { + return Buffer.from('/usr/local/bin/gh'); + } + if (cmd === 'gh auth status') { + return Buffer.from('Logged in'); + } + return ''; + }); + + // gh resolves label names before creating the issue, so a repo without + // the label fails with no issue created + mockExecFileSync.mockImplementation((_cmd: string, args: string[]) => { + if (args.includes('--label')) { + const error: any = new Error('gh failed'); + error.status = 1; + error.stderr = Buffer.from( + 'could not add label: labels not found: feedback' + ); + throw error; + } + return `${issueUrl}\n`; + }); + + await feedbackCommand.execute('Test'); + + expect(mockExecFileSync).toHaveBeenCalledTimes(2); + + // First attempt asks for the label + expect(mockExecFileSync).toHaveBeenNthCalledWith( + 1, + 'gh', + expect.arrayContaining(['--label', 'feedback']), + expect.any(Object) + ); + + // Retry drops it + expect(mockExecFileSync).toHaveBeenNthCalledWith( + 2, + 'gh', + expect.not.arrayContaining(['--label']), + expect.any(Object) + ); + + // The feedback still lands as an issue, and the user is told the label + // was not applied + expect(consoleLogSpy).toHaveBeenCalledWith( + expect.stringContaining('Feedback submitted successfully') + ); + expect(consoleLogSpy).toHaveBeenCalledWith( + expect.stringContaining(issueUrl) + ); + expect(consoleLogSpy).toHaveBeenCalledWith( + expect.stringContaining("without the 'feedback' label") + ); + }); + + it('should preserve gh exit code when the unlabeled retry also fails', async () => { + mockExecSync.mockImplementation((cmd: string, options?: any) => { + if (cmd === 'which gh' || cmd === 'where gh') { + return Buffer.from('/usr/local/bin/gh'); + } + if (cmd === 'gh auth status') { + return Buffer.from('Logged in'); + } + return ''; + }); + + mockExecFileSync.mockImplementation((_cmd: string, args: string[]) => { + const error: any = new Error('gh failed'); + + if (args.includes('--label')) { + error.status = 1; + error.stderr = Buffer.from( + 'could not add label: labels not found: feedback' + ); + } else { + error.status = 4; + error.stderr = Buffer.from('Error: issues are disabled'); + } + + throw error; + }); + + await expect(feedbackCommand.execute('Test')).rejects.toThrow( + 'process.exit(4)' + ); + + expect(mockExecFileSync).toHaveBeenCalledTimes(2); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining('issues are disabled') + ); }); it('should handle quotes in title and body without escaping (no shell injection)', async () => {