fix(look-at): revert to sync prompt to fix race condition with async polling
df0b9f76 regressed look_at from synchronous prompt (session.prompt) to async prompt (session.promptAsync) + pollSessionUntilIdle polling. This introduced a race condition where the poller fires before the server registers the session as busy, causing it to return immediately with no messages available. Fix: restore promptSyncWithModelSuggestionRetry (blocking HTTP call) and remove polling entirely. Catch prompt errors gracefully and still attempt to fetch messages, since session.prompt may throw even on success.
This commit is contained in:
parent
687cc2386f
commit
13d960f3ca
@ -1,4 +1,4 @@
|
|||||||
import { describe, expect, test } from "bun:test"
|
import { describe, expect, test, mock } from "bun:test"
|
||||||
import type { ToolContext } from "@opencode-ai/plugin/tool"
|
import type { ToolContext } from "@opencode-ai/plugin/tool"
|
||||||
import { normalizeArgs, validateArgs, createLookAt } from "./tools"
|
import { normalizeArgs, validateArgs, createLookAt } from "./tools"
|
||||||
|
|
||||||
@ -111,16 +111,15 @@ describe("look-at tool", () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
describe("createLookAt error handling", () => {
|
describe("createLookAt error handling", () => {
|
||||||
// given promptAsync throws error
|
// given sync prompt throws and no messages available
|
||||||
// when LookAt tool executed
|
// when LookAt tool executed
|
||||||
// then returns error string immediately (no message fetch)
|
// then returns no-response error (fetches messages after catching prompt error)
|
||||||
test("returns error immediately when promptAsync fails", async () => {
|
test("returns no-response error when prompt fails and no messages exist", async () => {
|
||||||
const mockClient = {
|
const mockClient = {
|
||||||
session: {
|
session: {
|
||||||
get: async () => ({ data: { directory: "/project" } }),
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
create: async () => ({ data: { id: "ses_test_prompt_fail" } }),
|
create: async () => ({ data: { id: "ses_test_prompt_fail" } }),
|
||||||
promptAsync: async () => { throw new Error("Network connection failed") },
|
prompt: async () => { throw new Error("Network connection failed") },
|
||||||
status: async () => ({ data: {} }),
|
|
||||||
messages: async () => ({ data: [] }),
|
messages: async () => ({ data: [] }),
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@ -146,51 +145,10 @@ describe("look-at tool", () => {
|
|||||||
toolContext,
|
toolContext,
|
||||||
)
|
)
|
||||||
expect(result).toContain("Error")
|
expect(result).toContain("Error")
|
||||||
expect(result).toContain("Network connection failed")
|
expect(result).toContain("multimodal-looker")
|
||||||
})
|
})
|
||||||
|
|
||||||
// given promptAsync succeeds but status API fails (polling degrades gracefully)
|
// given sync prompt succeeds
|
||||||
// when LookAt tool executed
|
|
||||||
// then still attempts to fetch messages (graceful degradation)
|
|
||||||
test("fetches messages even when status API fails", async () => {
|
|
||||||
const mockClient = {
|
|
||||||
session: {
|
|
||||||
get: async () => ({ data: { directory: "/project" } }),
|
|
||||||
create: async () => ({ data: { id: "ses_test_poll_timeout" } }),
|
|
||||||
promptAsync: async () => ({}),
|
|
||||||
status: async () => ({ error: new Error("status unavailable") }),
|
|
||||||
messages: async () => ({
|
|
||||||
data: [
|
|
||||||
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "partial result" }] },
|
|
||||||
],
|
|
||||||
}),
|
|
||||||
},
|
|
||||||
}
|
|
||||||
|
|
||||||
const tool = createLookAt({
|
|
||||||
client: mockClient,
|
|
||||||
directory: "/project",
|
|
||||||
} as any)
|
|
||||||
|
|
||||||
const toolContext: ToolContext = {
|
|
||||||
sessionID: "parent-session",
|
|
||||||
messageID: "parent-message",
|
|
||||||
agent: "sisyphus",
|
|
||||||
directory: "/project",
|
|
||||||
worktree: "/project",
|
|
||||||
abort: new AbortController().signal,
|
|
||||||
metadata: () => {},
|
|
||||||
ask: async () => {},
|
|
||||||
}
|
|
||||||
|
|
||||||
const result = await tool.execute(
|
|
||||||
{ file_path: "/test/file.png", goal: "analyze" },
|
|
||||||
toolContext,
|
|
||||||
)
|
|
||||||
expect(result).toBe("partial result")
|
|
||||||
})
|
|
||||||
|
|
||||||
// given promptAsync succeeds and session becomes idle
|
|
||||||
// when LookAt tool executed and no assistant message found
|
// when LookAt tool executed and no assistant message found
|
||||||
// then returns error about no response
|
// then returns error about no response
|
||||||
test("returns error when no assistant message after successful prompt", async () => {
|
test("returns error when no assistant message after successful prompt", async () => {
|
||||||
@ -198,8 +156,7 @@ describe("look-at tool", () => {
|
|||||||
session: {
|
session: {
|
||||||
get: async () => ({ data: { directory: "/project" } }),
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
create: async () => ({ data: { id: "ses_test_no_msg" } }),
|
create: async () => ({ data: { id: "ses_test_no_msg" } }),
|
||||||
promptAsync: async () => ({}),
|
prompt: async () => ({}),
|
||||||
status: async () => ({ data: {} }),
|
|
||||||
messages: async () => ({ data: [] }),
|
messages: async () => ({ data: [] }),
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@ -236,8 +193,7 @@ describe("look-at tool", () => {
|
|||||||
session: {
|
session: {
|
||||||
get: async () => ({ data: { directory: "/project" } }),
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
create: async () => ({ error: "Internal server error" }),
|
create: async () => ({ error: "Internal server error" }),
|
||||||
promptAsync: async () => ({}),
|
prompt: async () => ({}),
|
||||||
status: async () => ({ data: {} }),
|
|
||||||
messages: async () => ({ data: [] }),
|
messages: async () => ({ data: [] }),
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@ -270,8 +226,8 @@ describe("look-at tool", () => {
|
|||||||
describe("createLookAt model passthrough", () => {
|
describe("createLookAt model passthrough", () => {
|
||||||
// given multimodal-looker agent has resolved model info
|
// given multimodal-looker agent has resolved model info
|
||||||
// when LookAt tool executed
|
// when LookAt tool executed
|
||||||
// then model info should be passed to promptAsync
|
// then model info should be passed to sync prompt
|
||||||
test("passes multimodal-looker model to promptAsync when available", async () => {
|
test("passes multimodal-looker model to sync prompt when available", async () => {
|
||||||
let promptBody: any
|
let promptBody: any
|
||||||
|
|
||||||
const mockClient = {
|
const mockClient = {
|
||||||
@ -289,11 +245,10 @@ describe("look-at tool", () => {
|
|||||||
session: {
|
session: {
|
||||||
get: async () => ({ data: { directory: "/project" } }),
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
create: async () => ({ data: { id: "ses_model_passthrough" } }),
|
create: async () => ({ data: { id: "ses_model_passthrough" } }),
|
||||||
promptAsync: async (input: any) => {
|
prompt: async (input: any) => {
|
||||||
promptBody = input.body
|
promptBody = input.body
|
||||||
return { data: {} }
|
return { data: {} }
|
||||||
},
|
},
|
||||||
status: async () => ({ data: {} }),
|
|
||||||
messages: async () => ({
|
messages: async () => ({
|
||||||
data: [
|
data: [
|
||||||
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "done" }] },
|
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "done" }] },
|
||||||
@ -330,10 +285,154 @@ describe("look-at tool", () => {
|
|||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
describe("createLookAt sync prompt (race condition fix)", () => {
|
||||||
|
// given look_at needs response immediately after prompt returns
|
||||||
|
// when tool is executed
|
||||||
|
// then must use synchronous prompt (session.prompt), NOT async (session.promptAsync)
|
||||||
|
test("uses synchronous prompt to avoid race condition with polling", async () => {
|
||||||
|
const syncPrompt = mock(async () => ({}))
|
||||||
|
const asyncPrompt = mock(async () => ({}))
|
||||||
|
const statusFn = mock(async () => ({ data: {} }))
|
||||||
|
|
||||||
|
const mockClient = {
|
||||||
|
app: {
|
||||||
|
agents: async () => ({ data: [] }),
|
||||||
|
},
|
||||||
|
session: {
|
||||||
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
|
create: async () => ({ data: { id: "ses_sync_test" } }),
|
||||||
|
prompt: syncPrompt,
|
||||||
|
promptAsync: asyncPrompt,
|
||||||
|
status: statusFn,
|
||||||
|
messages: async () => ({
|
||||||
|
data: [
|
||||||
|
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "result" }] },
|
||||||
|
],
|
||||||
|
}),
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
const tool = createLookAt({
|
||||||
|
client: mockClient,
|
||||||
|
directory: "/project",
|
||||||
|
} as any)
|
||||||
|
|
||||||
|
const toolContext: ToolContext = {
|
||||||
|
sessionID: "parent-session",
|
||||||
|
messageID: "parent-message",
|
||||||
|
agent: "sisyphus",
|
||||||
|
directory: "/project",
|
||||||
|
worktree: "/project",
|
||||||
|
abort: new AbortController().signal,
|
||||||
|
metadata: () => {},
|
||||||
|
ask: async () => {},
|
||||||
|
}
|
||||||
|
|
||||||
|
const result = await tool.execute(
|
||||||
|
{ file_path: "/test/file.png", goal: "analyze" },
|
||||||
|
toolContext,
|
||||||
|
)
|
||||||
|
|
||||||
|
expect(result).toBe("result")
|
||||||
|
expect(syncPrompt).toHaveBeenCalledTimes(1)
|
||||||
|
expect(asyncPrompt).not.toHaveBeenCalled()
|
||||||
|
expect(statusFn).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
// given sync prompt throws (JSON parse error even on success)
|
||||||
|
// when tool is executed
|
||||||
|
// then catches error gracefully and still fetches messages
|
||||||
|
test("catches sync prompt errors and still fetches messages", async () => {
|
||||||
|
const mockClient = {
|
||||||
|
app: {
|
||||||
|
agents: async () => ({ data: [] }),
|
||||||
|
},
|
||||||
|
session: {
|
||||||
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
|
create: async () => ({ data: { id: "ses_sync_error" } }),
|
||||||
|
prompt: async () => { throw new Error("JSON parse error") },
|
||||||
|
promptAsync: async () => ({}),
|
||||||
|
status: async () => ({ data: {} }),
|
||||||
|
messages: async () => ({
|
||||||
|
data: [
|
||||||
|
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "result despite error" }] },
|
||||||
|
],
|
||||||
|
}),
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
const tool = createLookAt({
|
||||||
|
client: mockClient,
|
||||||
|
directory: "/project",
|
||||||
|
} as any)
|
||||||
|
|
||||||
|
const toolContext: ToolContext = {
|
||||||
|
sessionID: "parent-session",
|
||||||
|
messageID: "parent-message",
|
||||||
|
agent: "sisyphus",
|
||||||
|
directory: "/project",
|
||||||
|
worktree: "/project",
|
||||||
|
abort: new AbortController().signal,
|
||||||
|
metadata: () => {},
|
||||||
|
ask: async () => {},
|
||||||
|
}
|
||||||
|
|
||||||
|
const result = await tool.execute(
|
||||||
|
{ file_path: "/test/file.png", goal: "analyze" },
|
||||||
|
toolContext,
|
||||||
|
)
|
||||||
|
|
||||||
|
expect(result).toBe("result despite error")
|
||||||
|
})
|
||||||
|
|
||||||
|
// given sync prompt throws and no messages available
|
||||||
|
// when tool is executed
|
||||||
|
// then returns error about no response
|
||||||
|
test("returns no-response error when sync prompt fails and no messages", async () => {
|
||||||
|
const mockClient = {
|
||||||
|
app: {
|
||||||
|
agents: async () => ({ data: [] }),
|
||||||
|
},
|
||||||
|
session: {
|
||||||
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
|
create: async () => ({ data: { id: "ses_sync_no_msg" } }),
|
||||||
|
prompt: async () => { throw new Error("Connection refused") },
|
||||||
|
promptAsync: async () => ({}),
|
||||||
|
status: async () => ({ data: {} }),
|
||||||
|
messages: async () => ({ data: [] }),
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
const tool = createLookAt({
|
||||||
|
client: mockClient,
|
||||||
|
directory: "/project",
|
||||||
|
} as any)
|
||||||
|
|
||||||
|
const toolContext: ToolContext = {
|
||||||
|
sessionID: "parent-session",
|
||||||
|
messageID: "parent-message",
|
||||||
|
agent: "sisyphus",
|
||||||
|
directory: "/project",
|
||||||
|
worktree: "/project",
|
||||||
|
abort: new AbortController().signal,
|
||||||
|
metadata: () => {},
|
||||||
|
ask: async () => {},
|
||||||
|
}
|
||||||
|
|
||||||
|
const result = await tool.execute(
|
||||||
|
{ file_path: "/test/file.png", goal: "analyze" },
|
||||||
|
toolContext,
|
||||||
|
)
|
||||||
|
|
||||||
|
expect(result).toContain("Error")
|
||||||
|
expect(result).toContain("multimodal-looker")
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
describe("createLookAt with image_data", () => {
|
describe("createLookAt with image_data", () => {
|
||||||
// given base64 image data is provided
|
// given base64 image data is provided
|
||||||
// when LookAt tool executed
|
// when LookAt tool executed
|
||||||
// then should send data URL to promptAsync
|
// then should send data URL to sync prompt
|
||||||
test("sends data URL when image_data provided", async () => {
|
test("sends data URL when image_data provided", async () => {
|
||||||
let promptBody: any
|
let promptBody: any
|
||||||
|
|
||||||
@ -344,11 +443,10 @@ describe("look-at tool", () => {
|
|||||||
session: {
|
session: {
|
||||||
get: async () => ({ data: { directory: "/project" } }),
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
create: async () => ({ data: { id: "ses_image_data_test" } }),
|
create: async () => ({ data: { id: "ses_image_data_test" } }),
|
||||||
promptAsync: async (input: any) => {
|
prompt: async (input: any) => {
|
||||||
promptBody = input.body
|
promptBody = input.body
|
||||||
return { data: {} }
|
return { data: {} }
|
||||||
},
|
},
|
||||||
status: async () => ({ data: {} }),
|
|
||||||
messages: async () => ({
|
messages: async () => ({
|
||||||
data: [
|
data: [
|
||||||
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "analyzed" }] },
|
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "analyzed" }] },
|
||||||
@ -398,11 +496,10 @@ describe("look-at tool", () => {
|
|||||||
session: {
|
session: {
|
||||||
get: async () => ({ data: { directory: "/project" } }),
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
create: async () => ({ data: { id: "ses_raw_base64_test" } }),
|
create: async () => ({ data: { id: "ses_raw_base64_test" } }),
|
||||||
promptAsync: async (input: any) => {
|
prompt: async (input: any) => {
|
||||||
promptBody = input.body
|
promptBody = input.body
|
||||||
return { data: {} }
|
return { data: {} }
|
||||||
},
|
},
|
||||||
status: async () => ({ data: {} }),
|
|
||||||
messages: async () => ({
|
messages: async () => ({
|
||||||
data: [
|
data: [
|
||||||
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "analyzed" }] },
|
{ info: { role: "assistant", time: { created: 1 } }, parts: [{ type: "text", text: "analyzed" }] },
|
||||||
|
|||||||
@ -3,8 +3,7 @@ import { pathToFileURL } from "node:url"
|
|||||||
import { tool, type PluginInput, type ToolDefinition } from "@opencode-ai/plugin"
|
import { tool, type PluginInput, type ToolDefinition } from "@opencode-ai/plugin"
|
||||||
import { LOOK_AT_DESCRIPTION, MULTIMODAL_LOOKER_AGENT } from "./constants"
|
import { LOOK_AT_DESCRIPTION, MULTIMODAL_LOOKER_AGENT } from "./constants"
|
||||||
import type { LookAtArgs } from "./types"
|
import type { LookAtArgs } from "./types"
|
||||||
import { log, promptWithModelSuggestionRetry } from "../../shared"
|
import { log, promptSyncWithModelSuggestionRetry } from "../../shared"
|
||||||
import { pollSessionUntilIdle } from "./session-poller"
|
|
||||||
import { extractLatestAssistantText } from "./assistant-message-extractor"
|
import { extractLatestAssistantText } from "./assistant-message-extractor"
|
||||||
import type { LookAtArgsWithAlias } from "./look-at-arguments"
|
import type { LookAtArgsWithAlias } from "./look-at-arguments"
|
||||||
import { normalizeArgs, validateArgs } from "./look-at-arguments"
|
import { normalizeArgs, validateArgs } from "./look-at-arguments"
|
||||||
@ -106,9 +105,9 @@ Original error: ${createResult.error}`
|
|||||||
|
|
||||||
const { agentModel, agentVariant } = await resolveMultimodalLookerAgentMetadata(ctx)
|
const { agentModel, agentVariant } = await resolveMultimodalLookerAgentMetadata(ctx)
|
||||||
|
|
||||||
log(`[look_at] Sending async prompt with ${isBase64Input ? "base64 image" : "file"} to session ${sessionID}`)
|
log(`[look_at] Sending prompt with ${isBase64Input ? "base64 image" : "file"} to session ${sessionID}`)
|
||||||
try {
|
try {
|
||||||
await promptWithModelSuggestionRetry(ctx.client, {
|
await promptSyncWithModelSuggestionRetry(ctx.client, {
|
||||||
path: { id: sessionID },
|
path: { id: sessionID },
|
||||||
body: {
|
body: {
|
||||||
agent: MULTIMODAL_LOOKER_AGENT,
|
agent: MULTIMODAL_LOOKER_AGENT,
|
||||||
@ -127,15 +126,7 @@ Original error: ${createResult.error}`
|
|||||||
},
|
},
|
||||||
})
|
})
|
||||||
} catch (promptError) {
|
} catch (promptError) {
|
||||||
log(`[look_at] promptAsync error:`, promptError)
|
log(`[look_at] Prompt error (ignored, will still fetch messages):`, promptError)
|
||||||
return `Error: Failed to send prompt to multimodal-looker agent: ${promptError instanceof Error ? promptError.message : String(promptError)}`
|
|
||||||
}
|
|
||||||
|
|
||||||
log(`[look_at] Polling session ${sessionID} until idle...`)
|
|
||||||
try {
|
|
||||||
await pollSessionUntilIdle(ctx.client, sessionID, { pollIntervalMs: 500, timeoutMs: 120_000 })
|
|
||||||
} catch (pollError) {
|
|
||||||
log(`[look_at] Polling error (will still try to fetch messages):`, pollError)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
log(`[look_at] Fetching messages from session ${sessionID}...`)
|
log(`[look_at] Fetching messages from session ${sessionID}...`)
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user