-
Notifications
You must be signed in to change notification settings - Fork 321
fix(feedback): surface provider binary + error when Send Feedback spawn fails #1067
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
base: rc
Are you sure you want to change the base?
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 |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /** | ||
| * Tests for feedbackConversation.ts | ||
| * | ||
| * Focus: provider-startup failures must surface an actionable error (the | ||
| * resolved binary path + the provider's own output) instead of a generic | ||
| * "something went wrong". This is the failure users hit when multiple Codex | ||
| * installs are present and the wrong one (e.g. a codex-multi-auth wrapper or a | ||
| * shadowed nvm binary) gets auto-selected and can't start. See issue #1064. | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
|
|
||
| // Mock window.maestro (mirrors inlineWizardConversation.test.ts pattern) | ||
| const mockMaestro = { | ||
| agents: { | ||
| get: vi.fn(), | ||
| }, | ||
| process: { | ||
| spawn: vi.fn(), | ||
| onData: vi.fn(() => vi.fn()), | ||
| onExit: vi.fn(() => vi.fn()), | ||
| onThinkingChunk: vi.fn(() => vi.fn()), | ||
| kill: vi.fn(), | ||
| }, | ||
| }; | ||
|
|
||
| vi.stubGlobal('window', { maestro: mockMaestro }); | ||
|
|
||
| // Import after mocking | ||
| import { FeedbackConversationManager } from '../../../renderer/services/feedbackConversation'; | ||
|
|
||
| // Flush microtasks + the deferred spawn so the process listeners are registered. | ||
| const tick = () => new Promise((resolve) => setTimeout(resolve, 0)); | ||
|
|
||
| describe('FeedbackConversationManager - provider failure surfacing', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mockMaestro.process.spawn.mockResolvedValue(undefined); | ||
| mockMaestro.process.onData.mockImplementation(() => vi.fn()); | ||
| mockMaestro.process.onExit.mockImplementation(() => vi.fn()); | ||
| }); | ||
|
|
||
| it('spawns the resolved binary and reports its path + output when Codex exits non-zero', async () => { | ||
| // Mimics jeffscottward's box: detection resolves a specific multi-auth | ||
| // wrapper among several Codex installs. | ||
| const codexBinary = '/Users/jeff/.nvm/versions/node/v25.3.0/bin/codex-multi-auth-codex'; | ||
| mockMaestro.agents.get.mockResolvedValue({ | ||
| id: 'codex', | ||
| name: 'OpenAI Codex', | ||
| available: true, | ||
| command: 'codex', | ||
| path: codexBinary, | ||
| args: [], | ||
| }); | ||
|
|
||
| const manager = new FeedbackConversationManager(); | ||
| const sessionId = manager.start({ agentType: 'codex', systemPrompt: 'sys' }); | ||
|
|
||
| const onError = vi.fn(); | ||
| const responsePromise = manager.sendMessage('it broke', [], { onError }); | ||
|
|
||
| await tick(); | ||
|
|
||
| // The spawn must use the resolved binary, not the bare command name. | ||
| expect(mockMaestro.process.spawn).toHaveBeenCalledTimes(1); | ||
| expect(mockMaestro.process.spawn.mock.calls[0][0].command).toBe(codexBinary); | ||
|
|
||
| // Feed the provider's stderr, then simulate a non-zero exit. | ||
| const dataCb = mockMaestro.process.onData.mock.calls[0][0]; | ||
| dataCb(sessionId, 'Error: not logged in. Run `codex login` first.\n'); | ||
| const exitCb = mockMaestro.process.onExit.mock.calls[0][0]; | ||
| exitCb(sessionId, 1); | ||
|
|
||
| const response = await responsePromise; | ||
|
|
||
| // User-facing message names the exact binary and the underlying error - | ||
| // not the old generic string. | ||
| expect(response.message).toContain(codexBinary); | ||
| expect(response.message).toContain('not logged in'); | ||
| expect(response.message).not.toBe( | ||
| 'Something went wrong processing your message. Please try again.' | ||
| ); | ||
| expect(onError).toHaveBeenCalledWith(expect.stringContaining('not logged in')); | ||
| }); | ||
|
|
||
| it('still names the binary when the failed provider printed nothing', async () => { | ||
| const codexBinary = '/opt/homebrew/bin/codex'; | ||
| mockMaestro.agents.get.mockResolvedValue({ | ||
| id: 'codex', | ||
| name: 'OpenAI Codex', | ||
| available: true, | ||
| command: 'codex', | ||
| path: codexBinary, | ||
| args: [], | ||
| }); | ||
|
|
||
| const manager = new FeedbackConversationManager(); | ||
| const sessionId = manager.start({ agentType: 'codex', systemPrompt: 'sys' }); | ||
| const responsePromise = manager.sendMessage('hi', []); | ||
| await tick(); | ||
|
|
||
| const exitCb = mockMaestro.process.onExit.mock.calls[0][0]; | ||
| exitCb(sessionId, 127); | ||
|
|
||
| const response = await responsePromise; | ||
| expect(response.message).toContain(codexBinary); | ||
| expect(response.message).toContain('127'); | ||
| }); | ||
|
|
||
| it('throws with the resolved binary path when the provider is detected but not runnable', async () => { | ||
| const codexBinary = '/Users/jeff/.nvm/versions/node/v24.15.0/bin/codex'; | ||
| mockMaestro.agents.get.mockResolvedValue({ | ||
| id: 'codex', | ||
| name: 'OpenAI Codex', | ||
| available: false, | ||
| command: 'codex', | ||
| path: codexBinary, | ||
| args: [], | ||
| }); | ||
|
|
||
| const manager = new FeedbackConversationManager(); | ||
| manager.start({ agentType: 'codex', systemPrompt: 'sys' }); | ||
|
|
||
| await expect(manager.sendMessage('hi', [])).rejects.toThrow(codexBinary); | ||
| // An unavailable provider must not be spawned. | ||
| expect(mockMaestro.process.spawn).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
|
|
||
| import type { ToolType } from '../types'; | ||
| import { getStdinFlags } from '../utils/spawnHelpers'; | ||
| import { stripAnsiCodes } from '../../shared/stringUtils'; | ||
|
|
||
| // ============================================================================ | ||
| // Types | ||
|
|
@@ -165,6 +166,25 @@ function normalizeResponse(raw: any): FeedbackParsedResponse { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Distil a failed provider's raw output into a short, human-readable error | ||
| * detail. Strips ANSI, drops blank lines, and keeps the tail - where CLIs | ||
| * print the actual failure (auth prompts, "command not found", stack traces) - | ||
| * capped to a sane length. Lets the feedback UI surface the real cause instead | ||
| * of a generic "something went wrong", e.g. when the wrong Codex binary is | ||
| * selected among multiple installs. | ||
| */ | ||
| function summarizeProcessFailure(output: string): string { | ||
| const cleaned = stripAnsiCodes(output) | ||
| .split('\n') | ||
| .map((line) => line.trimEnd()) | ||
| .filter((line) => line.trim().length > 0); | ||
| if (cleaned.length === 0) return ''; | ||
| const tail = cleaned.slice(-8).join('\n'); | ||
| const MAX = 600; | ||
| return tail.length > MAX ? `...${tail.slice(-MAX)}` : tail; | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // FeedbackConversationManager | ||
| // ============================================================================ | ||
|
|
@@ -210,12 +230,21 @@ export class FeedbackConversationManager { | |
|
|
||
| const agent = await window.maestro.agents.get(this.agentType); | ||
| if (!agent) { | ||
| throw new Error(`Agent ${this.agentType} not found`); | ||
| throw new Error(`The ${this.agentType} provider could not be found.`); | ||
| } | ||
|
|
||
| // The binary Maestro resolved for this provider. Surfaced in every | ||
| // failure path below so the user can tell which install was used - | ||
| // critical when multiple Codex binaries (incl. wrappers) are present. | ||
| const binaryPath = agent.path || agent.command; | ||
|
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.
|
||
|
|
||
| const isRemote = this.sshRemoteConfig?.enabled && this.sshRemoteConfig?.remoteId; | ||
| if (!isRemote && !agent.available) { | ||
| throw new Error(`Agent ${this.agentType} is not available`); | ||
| throw new Error( | ||
| `The ${agent.name || this.agentType} provider isn't available. Maestro resolved its ` + | ||
| `binary to "${binaryPath}", but it reported as not runnable - check that it's installed, ` + | ||
| `on your PATH, and authenticated.` | ||
| ); | ||
| } | ||
|
|
||
| const prompt = this.buildPrompt(userMessage, history); | ||
|
|
@@ -267,18 +296,26 @@ export class FeedbackConversationManager { | |
| callbacks?.onComplete?.(response); | ||
| resolve(response); | ||
| } else { | ||
| const errorResponse = { | ||
| ...DEFAULT_FEEDBACK_RESPONSE, | ||
| message: 'Something went wrong processing your message. Please try again.', | ||
| }; | ||
| callbacks?.onError?.(`Agent exited with code ${code}`); | ||
| resolve(errorResponse); | ||
| // Surface the binary that was used plus whatever the provider | ||
| // printed before dying, instead of a generic "something went | ||
| // wrong". This is the common failure when the wrong Codex install | ||
| // is auto-selected and can't start (missing auth, shadowed path). | ||
| const detail = summarizeProcessFailure(this.outputBuffer); | ||
| const message = | ||
| `The ${agent.name || this.agentType} provider exited with code ${code} before it could respond.\n\n` + | ||
| `**Binary:** \`${binaryPath}\`\n\n` + | ||
| (detail | ||
| ? `**Output:**\n\n\`\`\`\n${detail}\n\`\`\`` | ||
|
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.
|
||
| : 'No output was captured - the binary may have failed to launch (wrong path, ' + | ||
| 'missing auth, or a shadowing install). If you have multiple installs, confirm the ' + | ||
| 'right one is selected.'); | ||
| callbacks?.onError?.(`Agent exited with code ${code}: ${detail || '(no output)'}`); | ||
| resolve({ ...DEFAULT_FEEDBACK_RESPONSE, message }); | ||
| } | ||
| }); | ||
|
|
||
| // Build args based on agent type | ||
| const argsForSpawn = this.buildArgsForAgent(agent); | ||
| const commandToUse = agent.path || agent.command; | ||
|
|
||
| // Get stdin flags for Windows | ||
| const isSshSession = Boolean(this.sshRemoteConfig?.enabled); | ||
|
|
@@ -293,7 +330,7 @@ export class FeedbackConversationManager { | |
| sessionId: currentSessionId, | ||
| toolType: this.agentType!, | ||
| cwd: '.', | ||
| command: commandToUse, | ||
| command: binaryPath, | ||
| args: argsForSpawn, | ||
| prompt, | ||
| ...stdinFlags, | ||
|
|
||
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.
Report the caught error to Sentry before surfacing it.
This
catchnow renderserror.messageto the user but never reports it. Unexpected failures frommanagerRef.current.sendMessageare silently swallowed here, unlike the sibling handlersagentDetect(Line 201) andstartConversation(Line 307), which both callcaptureException. Add the same reporting so production breakage stays visible while still showing the actionable detail.🛡️ Proposed fix
} catch (error) { + // Report so unexpected provider/runtime failures stay visible in + // production, while still surfacing an actionable detail to the user. + captureException(error, { extra: { source: 'FeedbackChatView.sendMessage' } }); // Surface the real reason (e.g. provider binary not found / not // runnable, with its resolved path) rather than a generic message, // so multi-install / auth problems are actionable. const detail = error instanceof Error && error.message ? error.message : 'Something went wrong. Please try again.';As per coding guidelines: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry... Use Sentry utilities (
captureException,captureMessage) fromsrc/utils/sentry.ts."📝 Committable suggestion
🤖 Prompt for AI Agents