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
5 changes: 5 additions & 0 deletions .changeset/feedback-missing-label-retry.md
Original file line number Diff line number Diff line change
@@ -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.
12 changes: 11 additions & 1 deletion openspec/specs/cli-feedback/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
105 changes: 77 additions & 28 deletions src/commands/feedback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() ?? '');
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* 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 createIssue(title: string, body: string, labels: string[]): string {
const args = [
'issue',
'create',
'--repo',
'Fission-AI/OpenSpec',
'--title',
title,
'--body',
body,
];

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 {
try {
const result = execFileSync(
'gh',
[
'issue',
'create',
'--repo',
'Fission-AI/OpenSpec',
'--title',
title,
'--body',
body,
'--label',
'feedback',
],
{ encoding: 'utf-8', stdio: 'pipe' }
);
let issueUrl: string;
let labelApplied = true;

const issueUrl = result.trim();
console.log(`\n✓ Feedback submitted successfully!`);
console.log(`Issue URL: ${issueUrl}\n`);
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"
);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Expand Down
148 changes: 142 additions & 6 deletions test/commands/feedback.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
Loading