-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: handle non-text response parts in GitHub action #6173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,6 +147,29 @@ export function parseGitHubRemote(url: string): { owner: string; repo: string } | |
| return { owner: match[1], repo: match[2] } | ||
| } | ||
|
|
||
| /** | ||
| * Extracts displayable text from assistant response parts. | ||
| * Returns null for tool-only or reasoning-only responses (signals summary needed). | ||
| * Throws for truly unusable responses (empty, step-start only, etc.). | ||
| */ | ||
| export function extractResponseText(parts: MessageV2.Part[]): string | null { | ||
| // Priority 1: Look for text parts | ||
| const textPart = parts.findLast((p) => p.type === "text") | ||
| if (textPart) return textPart.text | ||
|
|
||
| // Priority 2: Reasoning-only - return null to signal summary needed | ||
| const reasoningPart = parts.findLast((p) => p.type === "reasoning") | ||
| if (reasoningPart) return null | ||
|
|
||
| // Priority 3: Tool-only - return null to signal summary needed | ||
| const toolParts = parts.filter((p) => p.type === "tool" && p.state.status === "completed") | ||
| if (toolParts.length > 0) return null | ||
|
|
||
| // No usable parts - throw with debug info | ||
| const partTypes = parts.map((p) => p.type).join(", ") || "none" | ||
| throw new Error(`Failed to parse response. Part types found: [${partTypes}]`) | ||
| } | ||
|
|
||
| export const GithubCommand = cmd({ | ||
| command: "github", | ||
| describe: "manage GitHub agent", | ||
|
|
@@ -856,10 +879,41 @@ export const GithubRunCommand = cmd({ | |
| ) | ||
| } | ||
|
|
||
| const match = result.parts.findLast((p) => p.type === "text") | ||
| if (!match) throw new Error("Failed to parse the text response") | ||
| const text = extractResponseText(result.parts) | ||
| if (text) return text | ||
|
|
||
| // No text part (tool-only or reasoning-only) - ask agent to summarize | ||
| console.log("Requesting summary from agent...") | ||
| const summary = await SessionPrompt.prompt({ | ||
| sessionID: session.id, | ||
| messageID: Identifier.ascending("message"), | ||
| model: { | ||
| providerID, | ||
| modelID, | ||
| }, | ||
| tools: { "*": false }, // Disable all tools to force text response | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also note - we explicitly disable tools here as we only want to summarize. This feels right, but not sure if it's surprising to users.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that makes sense to me |
||
| parts: [ | ||
| { | ||
| id: Identifier.ascending("part"), | ||
| type: "text", | ||
| text: "Summarize the actions (tool calls & reasoning) you did for the user in 1-2 sentences.", | ||
| }, | ||
| ], | ||
| }) | ||
|
|
||
| if (summary.info.role === "assistant" && summary.info.error) { | ||
| console.error(summary.info) | ||
| throw new Error( | ||
| `${summary.info.error.name}: ${"message" in summary.info.error ? summary.info.error.message : ""}`, | ||
| ) | ||
| } | ||
|
|
||
| const summaryText = extractResponseText(summary.parts) | ||
| if (!summaryText) { | ||
| throw new Error("Failed to get summary from agent") | ||
| } | ||
|
|
||
| return match.text | ||
| return summaryText | ||
| } | ||
|
|
||
| async function getOidcToken() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| import { test, expect, describe } from "bun:test" | ||
| import { extractResponseText } from "../../src/cli/cmd/github" | ||
| import type { MessageV2 } from "../../src/session/message-v2" | ||
|
|
||
| // Helper to create minimal valid parts | ||
| function createTextPart(text: string): MessageV2.Part { | ||
| return { | ||
| id: "1", | ||
| sessionID: "s", | ||
| messageID: "m", | ||
| type: "text" as const, | ||
| text, | ||
| } | ||
| } | ||
|
|
||
| function createReasoningPart(text: string): MessageV2.Part { | ||
| return { | ||
| id: "1", | ||
| sessionID: "s", | ||
| messageID: "m", | ||
| type: "reasoning" as const, | ||
| text, | ||
| time: { start: 0 }, | ||
| } | ||
| } | ||
|
|
||
| function createToolPart(tool: string, title: string, status: "completed" | "running" = "completed"): MessageV2.Part { | ||
| if (status === "completed") { | ||
| return { | ||
| id: "1", | ||
| sessionID: "s", | ||
| messageID: "m", | ||
| type: "tool" as const, | ||
| callID: "c1", | ||
| tool, | ||
| state: { | ||
| status: "completed", | ||
| input: {}, | ||
| output: "", | ||
| title, | ||
| metadata: {}, | ||
| time: { start: 0, end: 1 }, | ||
| }, | ||
| } | ||
| } | ||
| return { | ||
| id: "1", | ||
| sessionID: "s", | ||
| messageID: "m", | ||
| type: "tool" as const, | ||
| callID: "c1", | ||
| tool, | ||
| state: { | ||
| status: "running", | ||
| input: {}, | ||
| time: { start: 0 }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| function createStepStartPart(): MessageV2.Part { | ||
| return { | ||
| id: "1", | ||
| sessionID: "s", | ||
| messageID: "m", | ||
| type: "step-start" as const, | ||
| } | ||
| } | ||
|
|
||
| describe("extractResponseText", () => { | ||
| test("returns text from text part", () => { | ||
| const parts = [createTextPart("Hello world")] | ||
| expect(extractResponseText(parts)).toBe("Hello world") | ||
| }) | ||
|
|
||
| test("returns last text part when multiple exist", () => { | ||
| const parts = [createTextPart("First"), createTextPart("Last")] | ||
| expect(extractResponseText(parts)).toBe("Last") | ||
| }) | ||
|
|
||
| test("returns text even when tool parts follow", () => { | ||
| const parts = [createTextPart("I'll help with that."), createToolPart("todowrite", "3 todos")] | ||
| expect(extractResponseText(parts)).toBe("I'll help with that.") | ||
| }) | ||
|
|
||
| test("returns null for reasoning-only response (signals summary needed)", () => { | ||
| const parts = [createReasoningPart("Let me think about this...")] | ||
| expect(extractResponseText(parts)).toBeNull() | ||
| }) | ||
|
|
||
| test("returns null for tool-only response (signals summary needed)", () => { | ||
| // This is the exact scenario from the bug report - todowrite with no text | ||
| const parts = [createToolPart("todowrite", "8 todos")] | ||
| expect(extractResponseText(parts)).toBeNull() | ||
| }) | ||
|
|
||
| test("returns null for multiple completed tools", () => { | ||
| const parts = [ | ||
| createToolPart("read", "src/file.ts"), | ||
| createToolPart("edit", "src/file.ts"), | ||
| createToolPart("bash", "bun test"), | ||
| ] | ||
| expect(extractResponseText(parts)).toBeNull() | ||
| }) | ||
|
|
||
| test("ignores running tool parts (throws since no completed tools)", () => { | ||
| const parts = [createToolPart("bash", "", "running")] | ||
| expect(() => extractResponseText(parts)).toThrow("Failed to parse response") | ||
| }) | ||
|
|
||
| test("throws with part types on empty array", () => { | ||
| expect(() => extractResponseText([])).toThrow("Part types found: [none]") | ||
| }) | ||
|
|
||
| test("throws with part types on unhandled parts", () => { | ||
| const parts = [createStepStartPart()] | ||
| expect(() => extractResponseText(parts)).toThrow("Part types found: [step-start]") | ||
| }) | ||
|
|
||
| test("prefers text over reasoning when both present", () => { | ||
| const parts = [createReasoningPart("Internal thinking..."), createTextPart("Final answer")] | ||
| expect(extractResponseText(parts)).toBe("Final answer") | ||
| }) | ||
|
|
||
| test("prefers text over tools when both present", () => { | ||
| const parts = [createToolPart("read", "src/file.ts"), createTextPart("Here's what I found")] | ||
| expect(extractResponseText(parts)).toBe("Here's what I found") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rekram1-node FYI