From 60f720c43acd94de7645ac8629c614ede4682b6a Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 20 Jul 2026 14:42:49 -0500 Subject: [PATCH] fix(feedback): submit feedback when the repo has no feedback label (#1396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openspec feedback` passed `--label feedback` unconditionally, but the repository does not define that label. gh resolves label names before creating the issue, so it failed with "could not add label: labels not found: feedback" on every invocation and the command exited non-zero, discarding the feedback the user had just composed. Retry once without the label when — and only when — gh's stderr reports that it could not add the label, and tell the user the label was not applied. Every other failure keeps its existing behavior: print gh's error and exit with gh's exit code, with no retry. Only stderr is matched, because the error message also embeds the command line, which carries the user's own feedback text. The cli-feedback spec gains a scenario for the unlabeled path, and its gh-failure scenario is narrowed to exclude it. The fallback scenarios are unchanged. Refs #1091 Co-authored-by: Claude Opus 4.8 (1M context) --- .changeset/feedback-missing-label-retry.md | 5 + openspec/specs/cli-feedback/spec.md | 12 +- src/commands/feedback.ts | 107 +++++++++++---- test/commands/feedback.test.ts | 148 ++++++++++++++++++++- 4 files changed, 236 insertions(+), 36 deletions(-) create mode 100644 .changeset/feedback-missing-label-retry.md 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 () => {