diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 049ccb1869a..d27a50a2250 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -31,6 +31,7 @@ - Fixed visible per-keystroke lag while searching in the `/resume` session picker. Literal matches now rank synchronously from a cached per-session haystack, fuzzy scoring runs in bounded background chunks that converge to the same ranking (large listings previously rebuilt a fuzzy index per token per session on every keystroke), and the prompt-history SQLite lookup — an FTS query plus a LIKE scan over every stored prompt — is debounced off the keystroke path. - Fixed compiled Linux binary extension loading when bundled web-search header generation cannot read `header-generator` data files from the build-time path. ([#5178](https://github.com/can1357/oh-my-pi/issues/5178)) +- Fixed the built-in advisor treating empty `stop` completions without advice as successful reviews, so silent provider failures now enter the advisor retry/drop path. ([#5212](https://github.com/can1357/oh-my-pi/issues/5212)) ## [16.4.4] - 2026-07-11 diff --git a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts index 179756d8052..487852b178b 100644 --- a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts +++ b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts @@ -1294,6 +1294,110 @@ describe("advisor", () => { expect(failures).toHaveLength(2); }); + it("treats an empty stop turn without advise as a failed advisor turn", async () => { + const promptInputs: string[] = []; + const turnErrors: unknown[] = []; + const failures: unknown[] = []; + const adviceNotes: string[] = []; + const rollbackCalls: number[] = []; + const lengthsBeforePrompt: number[] = []; + const events: string[] = []; + const state: { messages: AgentMessage[]; error?: string } = { messages: [] }; + let promptCalls = 0; + const agent: AdvisorAgent = { + prompt: async input => { + promptCalls++; + promptInputs.push(input); + lengthsBeforePrompt.push(state.messages.length); + events.push(`prompt:${promptCalls}`); + state.messages.push({ role: "user", content: input, timestamp: promptCalls * 2 - 1 } as AgentMessage); + state.messages.push({ + role: "assistant", + content: [], + api: "mock", + provider: "mock", + model: "mock-advisor", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + }, + stopReason: "stop", + timestamp: promptCalls * 2, + } as unknown as AgentMessage); + state.error = undefined; + }, + abort: () => {}, + reset: () => { + state.messages.length = 0; + state.error = undefined; + }, + rollbackTo: count => { + rollbackCalls.push(count); + state.messages.length = count; + state.error = undefined; + }, + state, + }; + const messages: AgentMessage[] = [{ role: "user", content: "aaa", timestamp: 1 } as AgentMessage]; + const host: AdvisorRuntimeHost = { + snapshotMessages: () => messages, + enqueueAdvice: note => adviceNotes.push(note), + onTurnError: error => { + turnErrors.push(error); + events.push(`hook:${error instanceof Error ? error.message : String(error)}`); + }, + notifyFailure: error => { + failures.push(error); + events.push(`notify:${error instanceof Error ? error.message : String(error)}`); + }, + }; + const runtime = new AdvisorRuntime(agent, host, 0); + + runtime.onTurnEnd(messages); + await runtime.waitForCatchup(1000, 1); + + expect(promptInputs).toHaveLength(3); + expect(promptInputs[0]).toContain("aaa"); + expect(promptInputs[1]).toBe(promptInputs[0]); + expect(promptInputs[2]).toBe(promptInputs[0]); + expect(lengthsBeforePrompt).toEqual([0, 0, 0]); + expect(rollbackCalls).toEqual([0, 0, 0]); + expect(turnErrors).toHaveLength(3); + const turnErrorMessages = turnErrors.map(error => + (error instanceof Error ? error.message : String(error)).toLowerCase(), + ); + for (const message of turnErrorMessages) { + expect(message).toContain("advisor"); + expect(message).toContain("empty"); + expect(message).toContain("response"); + } + expect(failures).toHaveLength(1); + const failure = failures[0]; + if (!(failure instanceof Error)) throw new Error("expected advisor failure error"); + expect(failure.message.toLowerCase()).toContain("advisor"); + expect(failure.message.toLowerCase()).toContain("empty"); + expect(failure.message.toLowerCase()).toContain("response"); + expect(events).toHaveLength(7); + expect(events[0]).toBe("prompt:1"); + expect(events[1].startsWith("hook:")).toBe(true); + expect(events[1].toLowerCase()).toContain("empty"); + expect(events[2]).toBe("prompt:2"); + expect(events[3].startsWith("hook:")).toBe(true); + expect(events[3].toLowerCase()).toContain("empty"); + expect(events[4]).toBe("prompt:3"); + expect(events[5].startsWith("hook:")).toBe(true); + expect(events[5].toLowerCase()).toContain("empty"); + expect(events[6].toLowerCase()).toContain("empty"); + expect(events[6].startsWith("notify:")).toBe(true); + expect(adviceNotes).toEqual([]); + expect(state.messages).toHaveLength(0); + expect(state.error).toBeUndefined(); + expect(runtime.backlog).toBe(0); + }); + it("calls onTurnError with state.error before retrying the batch", async () => { const promptInputs: string[] = []; const turnErrors: unknown[] = []; diff --git a/packages/coding-agent/src/advisor/runtime.ts b/packages/coding-agent/src/advisor/runtime.ts index 8c88f86ae41..bc978ee0707 100644 --- a/packages/coding-agent/src/advisor/runtime.ts +++ b/packages/coding-agent/src/advisor/runtime.ts @@ -352,6 +352,10 @@ export class AdvisorRuntime { // successful empty cycle. const promptError = this.agent.state.error; if (promptError) throw new Error(promptError); + const emptyResponseError = getAdvisorEmptyResponseError( + this.agent.state.messages.slice(messageSnapshot), + ); + if (emptyResponseError) throw emptyResponseError; success = true; this.#consecutiveFailures = 0; this.#failureNotified = false; @@ -405,6 +409,38 @@ export class AdvisorRuntime { } } +function getAdvisorEmptyResponseError(messages: readonly AgentMessage[]): Error | undefined { + let sawAssistant = false; + for (const message of messages) { + if (message.role !== "assistant") continue; + sawAssistant = true; + if (message.stopReason !== "stop") return undefined; + if (hasAdvisorResponseContent(message)) return undefined; + } + if (sawAssistant) return new Error("Advisor turn returned an empty stop response without advice"); + if (messages.length > 0) return new Error("Advisor turn ended without an assistant response"); + return undefined; +} + +function hasAdvisorResponseContent(message: AssistantMessage): boolean { + return message.content.some(block => { + switch (block.type) { + case "text": + return block.text.trim().length > 0; + case "thinking": + return block.thinking.trim().length > 0; + case "redactedThinking": + return block.data.length > 0; + case "toolCall": + return block.name.trim().length > 0; + case "fallback": + return false; + default: + return false; + } + }); +} + type TextualContent = string | readonly (TextContent | ImageContent)[]; function obfuscateTextualContent(obfuscator: SecretObfuscator, content: TextualContent): TextualContent {