mirror of
https://mirror.skon.top/github.com/code-yeongyu/oh-my-opencode
synced 2026-04-30 10:40:24 +08:00
Merge pull request #2816 from code-yeongyu/fix/keep-agent-with-explicit-model
fix: always keep agent with explicit model, robust port binding & writable dir fallback
This commit is contained in:
@@ -1668,7 +1668,7 @@ describe("BackgroundManager.resume model persistence", () => {
|
||||
// then - model should be passed in prompt body
|
||||
expect(promptCalls).toHaveLength(1)
|
||||
expect(promptCalls[0].body.model).toEqual({ providerID: "anthropic", modelID: "claude-sonnet-4-20250514" })
|
||||
expect("agent" in promptCalls[0].body).toBe(false)
|
||||
expect(promptCalls[0].body.agent).toBe("explore")
|
||||
})
|
||||
|
||||
test("should NOT pass model when task has no model (backward compatibility)", async () => {
|
||||
@@ -1832,7 +1832,7 @@ describe("BackgroundManager - Non-blocking Queue Integration", () => {
|
||||
expect(task2.status).toBe("pending")
|
||||
})
|
||||
|
||||
test("should omit agent when launch has model and keep agent without model", async () => {
|
||||
test("should keep agent when launch has model and keep agent without model", async () => {
|
||||
// given
|
||||
const promptBodies: Array<Record<string, unknown>> = []
|
||||
let resolveFirstPromptStarted: (() => void) | undefined
|
||||
@@ -1894,7 +1894,7 @@ describe("BackgroundManager - Non-blocking Queue Integration", () => {
|
||||
expect(taskWithoutModel.status).toBe("pending")
|
||||
expect(promptBodies).toHaveLength(2)
|
||||
expect(promptBodies[0].model).toEqual({ providerID: "anthropic", modelID: "claude-opus-4-6" })
|
||||
expect("agent" in promptBodies[0]).toBe(false)
|
||||
expect(promptBodies[0].agent).toBe("test-agent")
|
||||
expect(promptBodies[1].agent).toBe("test-agent")
|
||||
expect("model" in promptBodies[1]).toBe(false)
|
||||
})
|
||||
@@ -4752,6 +4752,53 @@ describe("BackgroundManager - tool permission spread order", () => {
|
||||
manager.shutdown()
|
||||
})
|
||||
|
||||
test("startTask keeps agent when explicit model is configured", async () => {
|
||||
//#given
|
||||
const promptCalls: Array<{ path: { id: string }; body: Record<string, unknown> }> = []
|
||||
const client = {
|
||||
session: {
|
||||
get: async () => ({ data: { directory: "/test/dir" } }),
|
||||
create: async () => ({ data: { id: "session-1" } }),
|
||||
promptAsync: async (args: { path: { id: string }; body: Record<string, unknown> }) => {
|
||||
promptCalls.push(args)
|
||||
return {}
|
||||
},
|
||||
},
|
||||
}
|
||||
const manager = new BackgroundManager({ client, directory: tmpdir() } as unknown as PluginInput)
|
||||
const task: BackgroundTask = {
|
||||
id: "task-explicit-model",
|
||||
status: "pending",
|
||||
queuedAt: new Date(),
|
||||
description: "test task",
|
||||
prompt: "test prompt",
|
||||
agent: "sisyphus-junior",
|
||||
parentSessionID: "parent-session",
|
||||
parentMessageID: "parent-message",
|
||||
model: { providerID: "openai", modelID: "gpt-5.4", variant: "medium" },
|
||||
}
|
||||
const input: import("./types").LaunchInput = {
|
||||
description: task.description,
|
||||
prompt: task.prompt,
|
||||
agent: task.agent,
|
||||
parentSessionID: task.parentSessionID,
|
||||
parentMessageID: task.parentMessageID,
|
||||
model: task.model,
|
||||
}
|
||||
|
||||
//#when
|
||||
await (manager as unknown as { startTask: (item: { task: BackgroundTask; input: import("./types").LaunchInput }) => Promise<void> })
|
||||
.startTask({ task, input })
|
||||
|
||||
//#then
|
||||
expect(promptCalls).toHaveLength(1)
|
||||
expect(promptCalls[0].body.agent).toBe("sisyphus-junior")
|
||||
expect(promptCalls[0].body.model).toEqual({ providerID: "openai", modelID: "gpt-5.4" })
|
||||
expect(promptCalls[0].body.variant).toBe("medium")
|
||||
|
||||
manager.shutdown()
|
||||
})
|
||||
|
||||
test("resume respects explore agent restrictions", async () => {
|
||||
//#given
|
||||
let capturedTools: Record<string, unknown> | undefined
|
||||
@@ -4796,4 +4843,48 @@ describe("BackgroundManager - tool permission spread order", () => {
|
||||
|
||||
manager.shutdown()
|
||||
})
|
||||
|
||||
test("resume keeps agent when explicit model is configured", async () => {
|
||||
//#given
|
||||
let promptCall: { path: { id: string }; body: Record<string, unknown> } | undefined
|
||||
const client = {
|
||||
session: {
|
||||
promptAsync: async (args: { path: { id: string }; body: Record<string, unknown> }) => {
|
||||
promptCall = args
|
||||
return {}
|
||||
},
|
||||
abort: async () => ({}),
|
||||
},
|
||||
}
|
||||
const manager = new BackgroundManager({ client, directory: tmpdir() } as unknown as PluginInput)
|
||||
const task: BackgroundTask = {
|
||||
id: "task-explicit-model-resume",
|
||||
sessionID: "session-3",
|
||||
parentSessionID: "parent-session",
|
||||
parentMessageID: "parent-message",
|
||||
description: "resume task",
|
||||
prompt: "resume prompt",
|
||||
agent: "explore",
|
||||
status: "completed",
|
||||
startedAt: new Date(),
|
||||
completedAt: new Date(),
|
||||
model: { providerID: "anthropic", modelID: "claude-sonnet-4-20250514" },
|
||||
}
|
||||
getTaskMap(manager).set(task.id, task)
|
||||
|
||||
//#when
|
||||
await manager.resume({
|
||||
sessionId: "session-3",
|
||||
prompt: "continue",
|
||||
parentSessionID: "parent-session",
|
||||
parentMessageID: "parent-message",
|
||||
})
|
||||
|
||||
//#then
|
||||
expect(promptCall).toBeDefined()
|
||||
expect(promptCall?.body.agent).toBe("explore")
|
||||
expect(promptCall?.body.model).toEqual({ providerID: "anthropic", modelID: "claude-sonnet-4-20250514" })
|
||||
|
||||
manager.shutdown()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -515,9 +515,7 @@ export class BackgroundManager {
|
||||
promptWithModelSuggestionRetry(this.client, {
|
||||
path: { id: sessionID },
|
||||
body: {
|
||||
// When a model is explicitly provided, omit the agent name so opencode's
|
||||
// built-in agent fallback chain does not override the user-specified model.
|
||||
...(launchModel ? {} : { agent: input.agent }),
|
||||
agent: input.agent,
|
||||
...(launchModel ? { model: launchModel } : {}),
|
||||
...(launchVariant ? { variant: launchVariant } : {}),
|
||||
system: input.skillContent,
|
||||
@@ -794,9 +792,7 @@ export class BackgroundManager {
|
||||
this.client.session.promptAsync({
|
||||
path: { id: existingTask.sessionID },
|
||||
body: {
|
||||
// When a model is explicitly provided, omit the agent name so opencode's
|
||||
// built-in agent fallback chain does not override the user-specified model.
|
||||
...(resumeModel ? {} : { agent: existingTask.agent }),
|
||||
agent: existingTask.agent,
|
||||
...(resumeModel ? { model: resumeModel } : {}),
|
||||
...(resumeVariant ? { variant: resumeVariant } : {}),
|
||||
tools: (() => {
|
||||
|
||||
@@ -64,4 +64,63 @@ describe("background-agent spawner.startTask", () => {
|
||||
{ permission: "question", action: "deny", pattern: "*" },
|
||||
])
|
||||
})
|
||||
|
||||
test("keeps agent when explicit model is configured", async () => {
|
||||
//#given
|
||||
const promptCalls: any[] = []
|
||||
|
||||
const client = {
|
||||
session: {
|
||||
get: async () => ({ data: { directory: "/parent/dir" } }),
|
||||
create: async () => ({ data: { id: "ses_child" } }),
|
||||
promptAsync: async (args?: any) => {
|
||||
promptCalls.push(args)
|
||||
return {}
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
const task = createTask({
|
||||
description: "Test task",
|
||||
prompt: "Do work",
|
||||
agent: "sisyphus-junior",
|
||||
parentSessionID: "ses_parent",
|
||||
parentMessageID: "msg_parent",
|
||||
model: { providerID: "openai", modelID: "gpt-5.4", variant: "medium" },
|
||||
})
|
||||
|
||||
const item = {
|
||||
task,
|
||||
input: {
|
||||
description: task.description,
|
||||
prompt: task.prompt,
|
||||
agent: task.agent,
|
||||
parentSessionID: task.parentSessionID,
|
||||
parentMessageID: task.parentMessageID,
|
||||
parentModel: task.parentModel,
|
||||
parentAgent: task.parentAgent,
|
||||
model: task.model,
|
||||
},
|
||||
}
|
||||
|
||||
const ctx = {
|
||||
client,
|
||||
directory: "/fallback",
|
||||
concurrencyManager: { release: () => {} },
|
||||
tmuxEnabled: false,
|
||||
onTaskError: () => {},
|
||||
}
|
||||
|
||||
//#when
|
||||
await startTask(item as any, ctx as any)
|
||||
|
||||
//#then
|
||||
expect(promptCalls).toHaveLength(1)
|
||||
expect(promptCalls[0]?.body?.agent).toBe("sisyphus-junior")
|
||||
expect(promptCalls[0]?.body?.model).toEqual({
|
||||
providerID: "openai",
|
||||
modelID: "gpt-5.4",
|
||||
})
|
||||
expect(promptCalls[0]?.body?.variant).toBe("medium")
|
||||
})
|
||||
})
|
||||
|
||||
@@ -135,9 +135,7 @@ export async function startTask(
|
||||
promptWithModelSuggestionRetry(client, {
|
||||
path: { id: sessionID },
|
||||
body: {
|
||||
// When a model is explicitly provided, omit the agent name so opencode's
|
||||
// built-in agent fallback chain does not override the user-specified model.
|
||||
...(launchModel ? {} : { agent: input.agent }),
|
||||
agent: input.agent,
|
||||
...(launchModel ? { model: launchModel } : {}),
|
||||
...(launchVariant ? { variant: launchVariant } : {}),
|
||||
system: input.skillContent,
|
||||
@@ -222,9 +220,7 @@ export async function resumeTask(
|
||||
client.session.promptAsync({
|
||||
path: { id: task.sessionID },
|
||||
body: {
|
||||
// When a model is explicitly provided, omit the agent name so opencode's
|
||||
// built-in agent fallback chain does not override the user-specified model.
|
||||
...(resumeModel ? {} : { agent: task.agent }),
|
||||
agent: task.agent,
|
||||
...(resumeModel ? { model: resumeModel } : {}),
|
||||
...(resumeVariant ? { variant: resumeVariant } : {}),
|
||||
tools: {
|
||||
|
||||
@@ -1,44 +1,112 @@
|
||||
import { afterEach, describe, expect, it } from "bun:test"
|
||||
import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"
|
||||
import { startCallbackServer, type CallbackServer } from "./callback-server"
|
||||
|
||||
const HOSTNAME = "127.0.0.1"
|
||||
const nativeFetch = Bun.fetch.bind(Bun)
|
||||
|
||||
function supportsRealSocketBinding(): boolean {
|
||||
try {
|
||||
const server = Bun.serve({
|
||||
port: 0,
|
||||
hostname: HOSTNAME,
|
||||
fetch: () => new Response("probe"),
|
||||
})
|
||||
server.stop(true)
|
||||
return true
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
const canBindRealSockets = supportsRealSocketBinding()
|
||||
|
||||
type MockServerState = {
|
||||
port: number
|
||||
stopped: boolean
|
||||
fetch: (request: Request) => Response | Promise<Response>
|
||||
}
|
||||
|
||||
describe("startCallbackServer", () => {
|
||||
let server: CallbackServer | null = null
|
||||
let serveSpy: ReturnType<typeof spyOn> | null = null
|
||||
let activeServer: MockServerState | null = null
|
||||
|
||||
async function request(url: string): Promise<Response> {
|
||||
if (canBindRealSockets) {
|
||||
return nativeFetch(url)
|
||||
}
|
||||
|
||||
if (!activeServer || activeServer.stopped) {
|
||||
throw new Error("Connection refused")
|
||||
}
|
||||
|
||||
return await activeServer.fetch(new Request(url))
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
if (canBindRealSockets) {
|
||||
return
|
||||
}
|
||||
|
||||
activeServer = null
|
||||
serveSpy = spyOn(Bun, "serve").mockImplementation((options: {
|
||||
port: number
|
||||
hostname?: string
|
||||
fetch: (request: Request) => Response | Promise<Response>
|
||||
}) => {
|
||||
const state: MockServerState = {
|
||||
port: options.port === 0 ? 19877 : options.port,
|
||||
stopped: false,
|
||||
fetch: options.fetch,
|
||||
}
|
||||
|
||||
const handle = {
|
||||
port: state.port,
|
||||
stop: (_force?: boolean) => {
|
||||
state.stopped = true
|
||||
if (activeServer === state) {
|
||||
activeServer = null
|
||||
}
|
||||
},
|
||||
}
|
||||
|
||||
activeServer = state
|
||||
return handle as ReturnType<typeof Bun.serve>
|
||||
})
|
||||
})
|
||||
|
||||
afterEach(async () => {
|
||||
server?.close()
|
||||
server = null
|
||||
// Allow time for port to be released before next test
|
||||
await Bun.sleep(10)
|
||||
|
||||
if (serveSpy) {
|
||||
serveSpy.mockRestore()
|
||||
serveSpy = null
|
||||
}
|
||||
activeServer = null
|
||||
|
||||
if (canBindRealSockets) {
|
||||
await Bun.sleep(10)
|
||||
}
|
||||
})
|
||||
|
||||
it("starts server and returns port", async () => {
|
||||
// given - no preconditions
|
||||
|
||||
// when
|
||||
server = await startCallbackServer()
|
||||
|
||||
// then
|
||||
expect(server.port).toBeGreaterThanOrEqual(19877)
|
||||
expect(typeof server.waitForCallback).toBe("function")
|
||||
expect(typeof server.close).toBe("function")
|
||||
})
|
||||
|
||||
it("resolves callback with code and state from query params", async () => {
|
||||
// given
|
||||
server = await startCallbackServer()
|
||||
const callbackUrl = `http://127.0.0.1:${server.port}/oauth/callback?code=test-code&state=test-state`
|
||||
const callbackUrl = `http://${HOSTNAME}:${server.port}/oauth/callback?code=test-code&state=test-state`
|
||||
|
||||
// when
|
||||
// Use Promise.all to ensure fetch and waitForCallback run concurrently
|
||||
// This prevents race condition where waitForCallback blocks before fetch starts
|
||||
const [result, response] = await Promise.all([
|
||||
server.waitForCallback(),
|
||||
nativeFetch(callbackUrl)
|
||||
request(callbackUrl),
|
||||
])
|
||||
|
||||
// then
|
||||
expect(result).toEqual({ code: "test-code", state: "test-state" })
|
||||
expect(response.status).toBe(200)
|
||||
const html = await response.text()
|
||||
@@ -46,25 +114,19 @@ describe("startCallbackServer", () => {
|
||||
})
|
||||
|
||||
it("returns 404 for non-callback routes", async () => {
|
||||
// given
|
||||
server = await startCallbackServer()
|
||||
|
||||
// when
|
||||
const response = await nativeFetch(`http://127.0.0.1:${server.port}/other`)
|
||||
const response = await request(`http://${HOSTNAME}:${server.port}/other`)
|
||||
|
||||
// then
|
||||
expect(response.status).toBe(404)
|
||||
})
|
||||
|
||||
it("returns 400 and rejects when code is missing", async () => {
|
||||
// given
|
||||
server = await startCallbackServer()
|
||||
const callbackRejection = server.waitForCallback().catch((e: Error) => e)
|
||||
const callbackRejection = server.waitForCallback().catch((error: Error) => error)
|
||||
|
||||
// when
|
||||
const response = await nativeFetch(`http://127.0.0.1:${server.port}/oauth/callback?state=s`)
|
||||
const response = await request(`http://${HOSTNAME}:${server.port}/oauth/callback?state=s`)
|
||||
|
||||
// then
|
||||
expect(response.status).toBe(400)
|
||||
const error = await callbackRejection
|
||||
expect(error).toBeInstanceOf(Error)
|
||||
@@ -72,14 +134,11 @@ describe("startCallbackServer", () => {
|
||||
})
|
||||
|
||||
it("returns 400 and rejects when state is missing", async () => {
|
||||
// given
|
||||
server = await startCallbackServer()
|
||||
const callbackRejection = server.waitForCallback().catch((e: Error) => e)
|
||||
const callbackRejection = server.waitForCallback().catch((error: Error) => error)
|
||||
|
||||
// when
|
||||
const response = await nativeFetch(`http://127.0.0.1:${server.port}/oauth/callback?code=c`)
|
||||
const response = await request(`http://${HOSTNAME}:${server.port}/oauth/callback?code=c`)
|
||||
|
||||
// then
|
||||
expect(response.status).toBe(400)
|
||||
const error = await callbackRejection
|
||||
expect(error).toBeInstanceOf(Error)
|
||||
@@ -87,18 +146,15 @@ describe("startCallbackServer", () => {
|
||||
})
|
||||
|
||||
it("close stops the server immediately", async () => {
|
||||
// given
|
||||
server = await startCallbackServer()
|
||||
const port = server.port
|
||||
|
||||
// when
|
||||
server.close()
|
||||
server = null
|
||||
|
||||
// then
|
||||
try {
|
||||
await nativeFetch(`http://127.0.0.1:${port}/oauth/callback?code=c&state=s`)
|
||||
expect(true).toBe(false)
|
||||
await request(`http://${HOSTNAME}:${port}/oauth/callback?code=c&state=s`)
|
||||
expect.unreachable("request should fail after close")
|
||||
} catch (error) {
|
||||
expect(error).toBeDefined()
|
||||
}
|
||||
|
||||
@@ -39,7 +39,7 @@ export async function findAvailablePort(startPort: number = DEFAULT_PORT): Promi
|
||||
}
|
||||
|
||||
export async function startCallbackServer(startPort: number = DEFAULT_PORT): Promise<CallbackServer> {
|
||||
const port = await findAvailablePort(startPort)
|
||||
const requestedPort = await findAvailablePort(startPort).catch(() => 0)
|
||||
|
||||
let resolveCallback: ((result: OAuthCallbackResult) => void) | null = null
|
||||
let rejectCallback: ((error: Error) => void) | null = null
|
||||
@@ -55,7 +55,7 @@ export async function startCallbackServer(startPort: number = DEFAULT_PORT): Pro
|
||||
}, TIMEOUT_MS)
|
||||
|
||||
const server = Bun.serve({
|
||||
port,
|
||||
port: requestedPort,
|
||||
hostname: "127.0.0.1",
|
||||
fetch(request: Request): Response {
|
||||
const url = new URL(request.url)
|
||||
@@ -93,9 +93,10 @@ export async function startCallbackServer(startPort: number = DEFAULT_PORT): Pro
|
||||
})
|
||||
},
|
||||
})
|
||||
const activePort = server.port ?? requestedPort
|
||||
|
||||
return {
|
||||
port,
|
||||
port: activePort,
|
||||
waitForCallback: () => callbackPromise,
|
||||
close: () => {
|
||||
clearTimeout(timeoutId)
|
||||
|
||||
@@ -1,5 +1,18 @@
|
||||
import * as path from "node:path"
|
||||
import * as os from "node:os"
|
||||
import { accessSync, constants, mkdirSync } from "node:fs"
|
||||
|
||||
function resolveWritableDirectory(preferredDir: string, fallbackSuffix: string): string {
|
||||
try {
|
||||
mkdirSync(preferredDir, { recursive: true })
|
||||
accessSync(preferredDir, constants.W_OK)
|
||||
return preferredDir
|
||||
} catch {
|
||||
const fallbackDir = path.join(os.tmpdir(), fallbackSuffix)
|
||||
mkdirSync(fallbackDir, { recursive: true })
|
||||
return fallbackDir
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns the user-level data directory.
|
||||
@@ -10,7 +23,8 @@ import * as os from "node:os"
|
||||
* including Windows, so we match that behavior exactly.
|
||||
*/
|
||||
export function getDataDir(): string {
|
||||
return process.env.XDG_DATA_HOME ?? path.join(os.homedir(), ".local", "share")
|
||||
const preferredDir = process.env.XDG_DATA_HOME ?? path.join(os.homedir(), ".local", "share")
|
||||
return resolveWritableDirectory(preferredDir, "opencode-data")
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -27,7 +41,8 @@ export function getOpenCodeStorageDir(): string {
|
||||
* - All platforms: XDG_CACHE_HOME or ~/.cache
|
||||
*/
|
||||
export function getCacheDir(): string {
|
||||
return process.env.XDG_CACHE_HOME ?? path.join(os.homedir(), ".cache")
|
||||
const preferredDir = process.env.XDG_CACHE_HOME ?? path.join(os.homedir(), ".cache")
|
||||
return resolveWritableDirectory(preferredDir, "opencode-cache")
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -165,6 +165,55 @@ bunDescribe("sendSyncPrompt", () => {
|
||||
bunExpect(promptArgs.body.tools.call_omo_agent).toBe(true)
|
||||
})
|
||||
|
||||
bunTest("includes agent alongside explicit category model", async () => {
|
||||
//#given
|
||||
const { sendSyncPrompt } = require("./sync-prompt-sender")
|
||||
|
||||
let promptArgs: any
|
||||
const promptAsync = bunMock(async (input: any) => {
|
||||
promptArgs = input
|
||||
return { data: {} }
|
||||
})
|
||||
|
||||
const mockClient = {
|
||||
session: {
|
||||
promptAsync,
|
||||
},
|
||||
}
|
||||
|
||||
const input = {
|
||||
sessionID: "test-session",
|
||||
agentToUse: "sisyphus-junior",
|
||||
args: {
|
||||
description: "test task",
|
||||
prompt: "test prompt",
|
||||
category: "quick",
|
||||
run_in_background: false,
|
||||
load_skills: [],
|
||||
},
|
||||
systemContent: undefined,
|
||||
categoryModel: {
|
||||
providerID: "openai",
|
||||
modelID: "gpt-5.4",
|
||||
variant: "medium",
|
||||
},
|
||||
toastManager: null,
|
||||
taskId: undefined,
|
||||
}
|
||||
|
||||
//#when
|
||||
await sendSyncPrompt(mockClient, input)
|
||||
|
||||
//#then
|
||||
bunExpect(promptAsync).toHaveBeenCalled()
|
||||
bunExpect(promptArgs.body.agent).toBe("sisyphus-junior")
|
||||
bunExpect(promptArgs.body.model).toEqual({
|
||||
providerID: "openai",
|
||||
modelID: "gpt-5.4",
|
||||
})
|
||||
bunExpect(promptArgs.body.variant).toBe("medium")
|
||||
})
|
||||
|
||||
bunTest("retries with promptSync for oracle when promptAsync fails with unexpected EOF", async () => {
|
||||
//#given
|
||||
const { sendSyncPrompt } = require("./sync-prompt-sender")
|
||||
|
||||
@@ -56,9 +56,7 @@ export async function sendSyncPrompt(
|
||||
const promptArgs = {
|
||||
path: { id: input.sessionID },
|
||||
body: {
|
||||
// When a custom model is configured, omit the agent name so opencode's
|
||||
// built-in agent fallback chain does not override the user-specified model.
|
||||
...(input.categoryModel ? {} : { agent: input.agentToUse }),
|
||||
agent: input.agentToUse,
|
||||
system: input.systemContent,
|
||||
tools,
|
||||
parts: [createInternalAgentTextPart(effectivePrompt)],
|
||||
|
||||
Reference in New Issue
Block a user