fix(acp): omit inherited max for Codex spawns (#132116)

This commit is contained in:
PollyBot13 2026-09-11 02:39:25 +02:00 • committed by GitHub
parent 80f7815a21
commit 0d679d7442
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 199 additions and 12 deletions

View file

@ -1489,6 +1489,91 @@ describe("AcpxRuntime fresh reset wrapper", () => {
},
);
it("drops inherited max only after resolving the maintained Codex ACP command", async () => {
const baseStore: TestSessionStore = {
load: vi.fn(async () => undefined),
save: vi.fn(async () => {}),
};
const { runtime, delegate } = makeRuntime(baseStore, {
agentRegistry: {
resolve: (agentName: string) => (agentName === "codex" ? CODEX_ACP_COMMAND : agentName),
list: () => ["codex", "openclaw"],
},
});
const ensure = vi.spyOn(delegate, "ensureSession").mockResolvedValue({
sessionKey: "agent:codex:acp:inherited-max",
backend: "acpx",
runtimeSessionName: "codex",
});
const handle = await runtime.ensureSession({
sessionKey: "agent:codex:acp:inherited-max",
agent: "codex",
mode: "persistent",
thinking: "max",
thinkingExplicit: false,
});
expect(readFirstEnsureSessionInput(ensure)).not.toHaveProperty("thinking");
expect(readFirstEnsureSessionInput(ensure)).not.toHaveProperty("thinkingExplicit");
expect(handle.appliedThinking).toEqual({ kind: "dropped" });
});
it("preserves inherited max for a custom non-Codex command registered as codex", async () => {
const baseStore: TestSessionStore = {
load: vi.fn(async () => undefined),
save: vi.fn(async () => {}),
};
const { runtime, delegate } = makeRuntime(baseStore, {
agentRegistry: {
resolve: (agentName: string) => (agentName === "codex" ? "custom-acp" : agentName),
list: () => ["codex", "openclaw"],
},
});
const ensure = vi.spyOn(delegate, "ensureSession").mockResolvedValue({
sessionKey: "agent:codex:acp:custom-command",
backend: "acpx",
runtimeSessionName: "codex",
});
const handle = await runtime.ensureSession({
sessionKey: "agent:codex:acp:custom-command",
agent: "codex",
mode: "persistent",
thinking: "max",
thinkingExplicit: false,
});
expect(readFirstEnsureSessionInput(ensure)).toMatchObject({ thinking: "max" });
expect(readFirstEnsureSessionInput(ensure)).not.toHaveProperty("thinkingExplicit");
expect(handle.appliedThinking).toBeUndefined();
});
it("keeps rejecting an explicit max request for the maintained Codex ACP command", async () => {
const baseStore: TestSessionStore = {
load: vi.fn(async () => undefined),
save: vi.fn(async () => {}),
};
const { runtime, delegate } = makeRuntime(baseStore, {
agentRegistry: {
resolve: (agentName: string) => (agentName === "codex" ? CODEX_ACP_COMMAND : agentName),
list: () => ["codex", "openclaw"],
},
});
const ensure = vi.spyOn(delegate, "ensureSession");
await expect(
runtime.ensureSession({
sessionKey: "agent:codex:acp:explicit-max",
agent: "codex",
mode: "persistent",
thinking: "max",
thinkingExplicit: true,
}),
).rejects.toMatchObject({ code: "ACP_INVALID_RUNTIME_OPTION" });
expect(ensure).not.toHaveBeenCalled();
});
it("starts Codex ACP without injecting a leaked non-openai default model", async () => {
const baseStore: TestSessionStore = {
load: vi.fn(async () => undefined),

View file

@ -633,7 +633,7 @@ function withAcpxSessionOptions(input: OpenClawRuntimeEnsureInput): AcpxDelegate
const existingOptions = (input as { sessionOptions?: SessionAgentOptions }).sessionOptions;
const model = input.model?.trim() || existingOptions?.model;
const sessionOptions = model ? { ...existingOptions, model } : existingOptions;
const { modelExplicit: _modelExplicit, ...rest } = input;
const { modelExplicit: _modelExplicit, thinkingExplicit: _thinkingExplicit, ...rest } = input;
return {
...rest,
...(sessionOptions ? { sessionOptions } : {}),
@ -1428,11 +1428,17 @@ export class AcpxRuntime implements CompleteAcpRuntime {
const input = { ...logicalInput, sessionKey: resolveAcpxSessionResource(logicalInput) };
const isCodexAcp =
normalizeAgentName(input.agent) === CODEX_ACP_AGENT_ID && isCodexAcpCommand(command);
const dropInheritedCodexMax =
isCodexAcp && input.thinking === "max" && input.thinkingExplicit === false;
const effectiveInput = dropInheritedCodexMax ? { ...input } : input;
if (dropInheritedCodexMax) {
delete effectiveInput.thinking;
}
const claudeModelOverride = isClaudeAcpCommand(command)
? normalizeClaudeAcpModelOverride(input.model)
: undefined;
const codexClassification = isCodexAcp
? classifyCodexAcpModelRequest(input.model, input.thinking)
? classifyCodexAcpModelRequest(effectiveInput.model, effectiveInput.thinking)
: undefined;
if (codexClassification?.kind === "unsupported" && input.modelExplicit) {
failUnsupportedCodexAcpModel(input.model ?? "");
@ -1445,7 +1451,7 @@ export class AcpxRuntime implements CompleteAcpRuntime {
classifiedCodexOverride && Object.keys(classifiedCodexOverride).length > 0
? classifiedCodexOverride
: undefined;
const requestedModel = input.model?.trim();
const requestedModel = effectiveInput.model?.trim();
const appliedModel: OpenClawRuntimeHandle["appliedModel"] =
isCodexAcp && requestedModel
? codexModelOverride?.model
@ -1453,10 +1459,10 @@ export class AcpxRuntime implements CompleteAcpRuntime {
: { kind: "dropped" }
: undefined;
const ensureInput = isCodexAcp
? withCodexSessionModel(input, codexModelOverride)
? withCodexSessionModel(effectiveInput, codexModelOverride)
: claudeModelOverride
? { ...input, model: claudeModelOverride }
: input;
? { ...effectiveInput, model: claudeModelOverride }
: effectiveInput;
const stableLaunchCommand =
codexModelOverride && command
? appendCodexAcpConfigOverrides(command, codexModelOverride)
@ -1484,7 +1490,12 @@ export class AcpxRuntime implements CompleteAcpRuntime {
: ensureDelegateSessionWithModelFallback(delegate, ensureInput),
}),
});
return { ...handle, ...logicalTarget, ...(appliedModel ? { appliedModel } : {}) };
return {
...handle,
...logicalTarget,
...(appliedModel ? { appliedModel } : {}),
...(dropInheritedCodexMax ? { appliedThinking: { kind: "dropped" as const } } : {}),
};
}
async *runTurn(input: Parameters<AcpRuntime["runTurn"]>[0]): AsyncIterable<AcpRuntimeEvent> {

View file

@ -41,6 +41,11 @@ export type AcpRuntimeHandle = {
* model before the first turn. Absent when the backend did not deviate from the request.
*/
appliedModel?: { kind: "applied"; model: string } | { kind: "dropped" };
/**
* Effective thinking value when the backend intentionally changes the request. `dropped`
* prevents an unsupported inherited default from being persisted and replayed.
*/
appliedThinking?: { kind: "applied"; thinking: string } | { kind: "dropped" };
};
export type AcpRuntimeEnsureInput = {
@ -62,6 +67,8 @@ export type AcpRuntimeEnsureInput = {
modelExplicit?: boolean;
/** Optional runtime thinking/reasoning override that must be available during session creation. */
thinking?: string;
/** Whether `thinking` was an explicit caller selection rather than an inherited default. */
thinkingExplicit?: boolean;
cwd?: string;
env?: Record<string, string>;
};

View file

@ -57,6 +57,41 @@ describe("AcpSessionManager initializeSession", () => {
});
});
it("forwards inherited thinking provenance and omits thinking dropped by the backend", async () => {
const runtimeState = createRuntime();
runtimeState.ensureSession.mockResolvedValueOnce({
sessionKey: "agent:codex:acp:session-inherited-max",
backend: "acpx",
runtimeSessionName: "codex",
appliedThinking: { kind: "dropped" },
});
hoisted.requireAcpRuntimeBackendMock.mockReturnValue({
id: "acpx",
runtime: runtimeState.runtime,
});
hoisted.upsertAcpSessionMetaMock.mockResolvedValue({
sessionKey: "agent:codex:acp:session-inherited-max",
storeSessionKey: "agent:codex:acp:session-inherited-max",
acp: readySessionMeta(),
});
const manager = new AcpSessionManager();
await manager.initializeSession({
cfg: baseCfg,
sessionKey: "agent:codex:acp:session-inherited-max",
agent: "codex",
mode: "persistent",
runtimeOptions: { thinking: "max" },
thinkingExplicit: false,
});
expectRecordFields(mockCallArg(runtimeState.ensureSession), {
thinking: "max",
thinkingExplicit: false,
});
expect(extractRuntimeOptionsFromUpserts()).toEqual([undefined]);
});
it("preserves runtimeOptions cwd when initializeSession cwd is omitted", async () => {
const runtimeState = createRuntime();
hoisted.requireAcpRuntimeBackendMock.mockReturnValue({

View file

@ -70,6 +70,9 @@ export async function runManagerInitializeSession(params: {
...(requestedModel ? { model: requestedModel } : {}),
...(requestedModel && input.modelExplicit ? { modelExplicit: true } : {}),
...(requestedThinking ? { thinking: requestedThinking } : {}),
...(requestedThinking && input.thinkingExplicit !== undefined
? { thinkingExplicit: input.thinkingExplicit }
: {}),
cwd: requestedCwd,
}),
fallbackCode: "ACP_SESSION_INIT_FAILED",
@ -84,6 +87,11 @@ export async function runManagerInitializeSession(params: {
? handle.appliedModel.model
: undefined
: requestedModel,
thinking: handle.appliedThinking
? handle.appliedThinking.kind === "applied"
? handle.appliedThinking.thinking
: undefined
: requestedThinking,
...(effectiveCwd ? { cwd: effectiveCwd } : {}),
});

View file

@ -60,6 +60,7 @@ export type AcpInitializeSessionInput = {
resumeSessionId?: string;
runtimeOptions?: Partial<AcpSessionRuntimeOptions>;
modelExplicit?: boolean;
thinkingExplicit?: boolean;
cwd?: string;
backendId?: string;
};

View file

@ -36,7 +36,6 @@ import { splitModelRef } from "./subagent-spawn-plan.js";
import { resolveSubagentThinkingOverride } from "./subagent-spawn-thinking.js";
const ACP_RUNTIME_TIMEOUT_MAX_SECONDS = 24 * 60 * 60;
export function resolveAcpSessionMode(mode: "run" | "session"): AcpRuntimeSessionMode {
return mode === "session" ? "persistent" : "oneshot";
}
@ -94,10 +93,16 @@ export function resolveAcpSpawnRuntimeOptions(params: {
thinking?: string;
runTimeoutSeconds?: number;
}):
| { ok: true; runtimeOptions?: AcpSpawnRuntimeOptions; modelExplicit: boolean }
| {
ok: true;
runtimeOptions?: AcpSpawnRuntimeOptions;
modelExplicit: boolean;
thinkingExplicit: boolean;
}
| { ok: false; error: string } {
const policyAgentId = params.configAgentId ?? params.targetAgentId;
const modelExplicit = normalizeOptionalString(params.model) !== undefined;
const thinkingExplicit = normalizeOptionalString(params.thinking) !== undefined;
const rawModel = resolveConfiguredSubagentSpawnModelSelection({
cfg: params.cfg,
agentId: policyAgentId,
@ -137,7 +142,6 @@ export function resolveAcpSpawnRuntimeOptions(params: {
});
}
}
const timeoutSeconds = resolveAcpRuntimeTimeoutSeconds(params.runTimeoutSeconds);
const runtimeOptions =
model || thinking || timeoutSeconds
@ -147,7 +151,7 @@ export function resolveAcpSpawnRuntimeOptions(params: {
...(timeoutSeconds ? { timeoutSeconds } : {}),
}
: undefined;
return { ok: true, runtimeOptions, modelExplicit };
return { ok: true, runtimeOptions, modelExplicit, thinkingExplicit };
}
export async function initializeAcpSpawnRuntime(params: {
@ -160,6 +164,7 @@ export async function initializeAcpSpawnRuntime(params: {
resumeSessionId?: string;
runtimeOptions?: AcpSpawnRuntimeOptions;
modelExplicit?: boolean;
thinkingExplicit?: boolean;
cwd?: string;
}): Promise<AcpSpawnInitializedRuntime> {
params.assertActive?.();
@ -194,6 +199,7 @@ export async function initializeAcpSpawnRuntime(params: {
resumeSessionId: params.resumeSessionId,
runtimeOptions: params.runtimeOptions,
modelExplicit: params.modelExplicit,
thinkingExplicit: params.thinkingExplicit,
cwd: params.cwd,
backendId: params.backendId,
});

View file

@ -1425,6 +1425,8 @@ describe("spawnAcpDirect", () => {
globalSubagentThinking?: ThinkLevel;
thinking?: ThinkLevel;
expectedThinking?: ThinkLevel;
expectedThinkingExplicit?: boolean;
backend?: string;
}>([
{
scenario: "configured primary model with global thinking default",
@ -1465,6 +1467,28 @@ describe("spawnAcpDirect", () => {
thinking: "high",
expectedThinking: "high",
},
{
scenario: "explicit max thinking for Codex",
globalSubagentThinking: "low",
thinking: "max",
expectedThinking: "max",
expectedThinkingExplicit: true,
},
{
scenario: "inherited max thinking for Codex",
model: "openai/gpt-5.6-sol",
globalSubagentThinking: "max",
expectedThinking: "max",
expectedThinkingExplicit: false,
},
{
scenario: "inherited max thinking for Codex on another ACP backend",
model: "openai/gpt-5.6-sol",
globalSubagentThinking: "max",
expectedThinking: "max",
expectedThinkingExplicit: false,
backend: "alternate",
},
{
scenario: "harness defaults without an owner or model override",
globalThinking: "high",
@ -1480,6 +1504,8 @@ describe("spawnAcpDirect", () => {
globalSubagentThinking,
thinking,
expectedThinking,
expectedThinkingExplicit,
backend,
}) => {
replaceSpawnConfig({
...createDefaultSpawnConfig(),
@ -1487,7 +1513,10 @@ describe("spawnAcpDirect", () => {
list: [
{
id: "codex-acp",
runtime: { type: "acp", acp: { agent: "codex" } },
runtime: {
type: "acp",
acp: { agent: "codex", ...(backend ? { backend } : {}) },
},
model,
thinkingDefault: ownerThinking,
subagents: { thinking: subagentThinking },
@ -1515,6 +1544,10 @@ describe("spawnAcpDirect", () => {
expectAcceptedSpawn(result);
expectInitializeSessionFields({
agent: "codex",
backendId: backend ?? "acpx",
...(expectedThinkingExplicit !== undefined
? { thinkingExplicit: expectedThinkingExplicit }
: {}),
runtimeOptions:
model || expectedThinking
? {

View file

@ -482,6 +482,7 @@ export async function spawnAcpDirect(
resumeSessionId: params.resumeSessionId,
runtimeOptions: runtimeOptionsResult.runtimeOptions,
modelExplicit: runtimeOptionsResult.modelExplicit,
thinkingExplicit: runtimeOptionsResult.thinkingExplicit,
cwd: runtimeCwd,
});
closeRuntimeOnFailure = initializedSession.initialized.closeRuntimeOnFailure;