Skip to content

Commit 5ff9ea7

Browse files
committed
fix: address code review findings in feedback-submit tool
- Fix misleading error: `gh auth status` catch now returns `gh_auth_check_failed` instead of `gh_not_installed` (gh IS installed at that point) - Fix fragile version check: use `startsWith("gh version")` instead of `includes("not found")` for cross-platform correctness - Add `trim().min(1)` validation to `title` and `description` zod schemas to reject empty/whitespace-only inputs - Check `issueResult.exitCode !== 0` before stdout URL check on issue creation - Restore `Bun.$` in `afterAll` to prevent mock leaking across test files - Remove trailing `$ARGUMENTS` from end of feedback.txt template (duplicated from Step 1 pre-fill logic; could confuse model) - Add 8 new tests: empty/whitespace validation, `gh_auth_check_failed` path, non-zero exitCode with stdout scenario, and updated "not found" test name
1 parent 51632e6 commit 5ff9ea7

3 files changed

Lines changed: 109 additions & 9 deletions

File tree

packages/opencode/src/altimate/tools/feedback-submit.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,11 @@ export const FeedbackSubmitTool = Tool.define("feedback_submit", {
1919
"Creates an issue with appropriate labels and metadata. " +
2020
"Requires the `gh` CLI to be installed and authenticated.",
2121
parameters: z.object({
22-
title: z.string().describe("A concise title for the feedback issue"),
22+
title: z.string().trim().min(1).describe("A concise title for the feedback issue"),
2323
category: z
2424
.enum(["bug", "feature", "improvement", "ux"])
2525
.describe("The category of feedback: bug, feature, improvement, or ux"),
26-
description: z.string().describe("Detailed description of the feedback"),
26+
description: z.string().trim().min(1).describe("Detailed description of the feedback"),
2727
include_context: z
2828
.boolean()
2929
.optional()
@@ -50,7 +50,7 @@ export const FeedbackSubmitTool = Tool.define("feedback_submit", {
5050
// ENOENT — gh binary not found on PATH
5151
return ghNotInstalled
5252
}
53-
if (!ghVersion || ghVersion.includes("not found")) {
53+
if (!ghVersion.trim().startsWith("gh version")) {
5454
return ghNotInstalled
5555
}
5656

@@ -59,7 +59,13 @@ export const FeedbackSubmitTool = Tool.define("feedback_submit", {
5959
try {
6060
authStatus = await Bun.$`gh auth status`.quiet().nothrow()
6161
} catch {
62-
return ghNotInstalled
62+
return {
63+
title: "Feedback submission failed",
64+
metadata: { error: "gh_auth_check_failed", issueUrl: "" },
65+
output:
66+
"Failed to verify `gh` authentication status. Please check your installation with:\n" +
67+
" `gh auth status`",
68+
}
6369
}
6470
if (authStatus.exitCode !== 0) {
6571
return {
@@ -114,7 +120,7 @@ export const FeedbackSubmitTool = Tool.define("feedback_submit", {
114120
const stdout = issueResult.stdout.toString().trim()
115121
const stderr = issueResult.stderr.toString().trim()
116122

117-
if (!stdout || !stdout.includes("github.com")) {
123+
if (issueResult.exitCode !== 0 || !stdout || !stdout.includes("github.com")) {
118124
const errorDetail = stderr || stdout || "No output from gh CLI"
119125
return {
120126
title: "Feedback submission failed",

packages/opencode/src/command/template/feedback.txt

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,3 @@ Ask the user to confirm. If they confirm, call the `feedback_submit` tool with:
4040
Step 4 — Show result:
4141

4242
After submission, display the created GitHub issue URL to the user so they can track it. Thank them for the feedback.
43-
44-
$ARGUMENTS

packages/opencode/test/tool/feedback-submit.test.ts

Lines changed: 98 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, test, beforeEach } from "bun:test"
1+
import { describe, expect, test, beforeEach, afterAll } from "bun:test"
22
import Bun from "bun"
33

44
// ---------------------------------------------------------------------------
@@ -33,6 +33,11 @@ function pushShellThrow() {
3333
// replacing it here means dynamically imported modules will pick up our mock.
3434
// ---------------------------------------------------------------------------
3535

36+
const originalBunShell = Bun.$
37+
afterAll(() => {
38+
Bun.$ = originalBunShell
39+
})
40+
3641
Bun.$ = function mockedShell(strings: TemplateStringsArray, ...values: any[]) {
3742
const parts: string[] = []
3843
strings.forEach((s, i) => {
@@ -155,6 +160,46 @@ describe("tool.feedback_submit", () => {
155160
}
156161
})
157162

163+
test("rejects empty title", async () => {
164+
const tool = await FeedbackSubmitTool.init()
165+
const result = tool.parameters.safeParse({
166+
title: "",
167+
category: "bug",
168+
description: "test",
169+
})
170+
expect(result.success).toBe(false)
171+
})
172+
173+
test("rejects whitespace-only title", async () => {
174+
const tool = await FeedbackSubmitTool.init()
175+
const result = tool.parameters.safeParse({
176+
title: " ",
177+
category: "bug",
178+
description: "test",
179+
})
180+
expect(result.success).toBe(false)
181+
})
182+
183+
test("rejects empty description", async () => {
184+
const tool = await FeedbackSubmitTool.init()
185+
const result = tool.parameters.safeParse({
186+
title: "Test",
187+
category: "bug",
188+
description: "",
189+
})
190+
expect(result.success).toBe(false)
191+
})
192+
193+
test("rejects whitespace-only description", async () => {
194+
const tool = await FeedbackSubmitTool.init()
195+
const result = tool.parameters.safeParse({
196+
title: "Test",
197+
category: "bug",
198+
description: " ",
199+
})
200+
expect(result.success).toBe(false)
201+
})
202+
158203
test("rejects missing title", async () => {
159204
const tool = await FeedbackSubmitTool.init()
160205
const result = tool.parameters.safeParse({
@@ -240,7 +285,7 @@ describe("tool.feedback_submit", () => {
240285
expect(result.output).toContain("gh auth login")
241286
})
242287

243-
test("returns error when gh --version output contains 'not found'", async () => {
288+
test("returns error when gh --version output does not start with 'gh version'", async () => {
244289
const tool = await FeedbackSubmitTool.init()
245290

246291
pushShellResult("command not found: gh")
@@ -280,6 +325,35 @@ describe("tool.feedback_submit", () => {
280325
})
281326
})
282327

328+
// -------------------------------------------------------------------------
329+
// gh auth status throws
330+
// -------------------------------------------------------------------------
331+
332+
describe("gh auth status throws", () => {
333+
test("returns gh_auth_check_failed (not gh_not_installed) when auth check throws", async () => {
334+
const tool = await FeedbackSubmitTool.init()
335+
336+
// gh --version succeeds
337+
pushShellResult("gh version 2.40.0")
338+
// gh auth status throws
339+
pushShellThrow()
340+
341+
const result = await tool.execute(
342+
{
343+
title: "Test",
344+
category: "bug" as const,
345+
description: "test",
346+
include_context: false,
347+
},
348+
ctx,
349+
)
350+
351+
expect(result.title).toBe("Feedback submission failed")
352+
expect(result.metadata.error).toBe("gh_auth_check_failed")
353+
expect(result.output).toContain("gh auth status")
354+
})
355+
})
356+
283357
// -------------------------------------------------------------------------
284358
// gh CLI not authenticated
285359
// -------------------------------------------------------------------------
@@ -392,6 +466,28 @@ describe("tool.feedback_submit", () => {
392466
expect(result.metadata.error).toBe("issue_creation_failed")
393467
})
394468

469+
test("returns failure when issue creation has non-zero exitCode even with stdout", async () => {
470+
const tool = await FeedbackSubmitTool.init()
471+
472+
pushShellResult("gh version 2.40.0")
473+
pushShellResult("Logged in", 0)
474+
// gh issue create exits non-zero (e.g. label doesn't exist) with partial output
475+
shellResults.push({ text: "https://github.com/AltimateAI/altimate-code/issues/1", exitCode: 1 })
476+
477+
const result = await tool.execute(
478+
{
479+
title: "Test",
480+
category: "bug" as const,
481+
description: "test",
482+
include_context: false,
483+
},
484+
ctx,
485+
)
486+
487+
expect(result.title).toBe("Feedback submission failed")
488+
expect(result.metadata.error).toBe("issue_creation_failed")
489+
})
490+
395491
test("returns failure when issue creation throws", async () => {
396492
const tool = await FeedbackSubmitTool.init()
397493

0 commit comments

Comments
 (0)