fix(advisor): clamped advisor default thinking against the resolved model

`#resolveAdvisorRuntimeDescriptors` hardcoded `ThinkingLevel.Medium` when
no thinking suffix was configured. For reasoning models with no
controllable effort surface (`devin-agent`: `reasoning: true`,
`thinking: undefined` — Cascade selects effort by routing to sibling
model ids, not a wire param), that default tripped
`requireSupportedEffort` on the first advisor prompt with an empty
`Supported efforts:` list, disabling the advisor session-wide.

Route the default through `resolveThinkingLevelForModel(model, level)`
which preserves explicit `off`, clamps a concrete effort into the
model's supported range, and returns `undefined` for reasoning models
without controllable efforts — falling back to `Inherit` so no effort is
sent while reasoning stays enabled. Matches the `auto`-path fix
(`clampAutoThinkingEffort`) and the Autonomous Memory clamp
(`clampThinkingLevelForModel`).

Fixes #4579
This commit is contained in:
roboomp
2026-07-05 00:46:01 +00:00
parent 38565c58cb
commit a1f60f6584
3 changed files with 160 additions and 1 deletions
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed the advisor being disabled for the entire session when the advisor role resolves to a reasoning model that exposes no controllable effort surface (Devin `devin/glm-5-2*`: `reasoning: true`, `thinking: undefined` — Cascade routes by sibling model id rather than a wire param). `#resolveAdvisorRuntimeDescriptors` in `packages/coding-agent/src/session/agent-session.ts` used to hardcode `ThinkingLevel.Medium`, which tripped `requireSupportedEffort` on the first advisor prompt with `Thinking effort medium is not supported by devin/glm-5-2. Supported efforts:` (empty list). The advisor descriptor now clamps the requested effort against the resolved model via `resolveThinkingLevelForModel` and forwards no explicit effort when the model has no controllable efforts — matching the `auto`-path fix (`clampAutoThinkingEffort`) and the Autonomous Memory stage fix (`clampThinkingLevelForModel`). Explicit `:off` still disables reasoning, and models that support `medium` (e.g. Anthropic) keep receiving it ([#4579](https://github.com/can1357/oh-my-pi/issues/4579)).
## [16.3.6] - 2026-07-04
### Changed
@@ -2339,7 +2339,19 @@ export class AgentSession {
model = sel.model;
thinkingLevel = concreteThinkingLevel(sel.thinkingLevel);
}
const advisorThinkingLevel = thinkingLevel ?? ThinkingLevel.Medium;
// Clamp the effort against the resolved model. Historically we defaulted
// to `ThinkingLevel.Medium` unconditionally, which threw at first stream
// on reasoning models that expose no controllable effort surface
// (e.g. `devin-agent`: Cascade routes by sibling model id, not a wire
// param; `getSupportedEfforts` returns `[]`). `resolveThinkingLevelForModel`
// preserves an explicit `off`, clamps a concrete effort into the model's
// supported range, and returns `undefined` for reasoning models without
// controllable efforts — for that case we forward `Inherit` so no effort
// is sent and reasoning stays enabled (matching the `auto`-path fix for
// Devin models via `clampAutoThinkingEffort`). See #4579.
const requestedLevel = thinkingLevel ?? ThinkingLevel.Medium;
const resolvedLevel = resolveThinkingLevelForModel(model, requestedLevel);
const advisorThinkingLevel: ThinkingLevel = resolvedLevel ?? ThinkingLevel.Inherit;
descriptors.push({
config,
name: config.name,
@@ -0,0 +1,143 @@
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test";
import * as path from "node:path";
import { Agent } from "@oh-my-pi/pi-agent-core";
import { Effort, type Model } from "@oh-my-pi/pi-ai";
import { getBundledModel } from "@oh-my-pi/pi-catalog/models";
import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry";
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session";
import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage";
import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager";
import { TempDir } from "@oh-my-pi/pi-utils";
// Regression for https://github.com/can1357/oh-my-pi/issues/4579.
//
// When the advisor role resolves to a reasoning model without a controllable
// effort surface (Devin `devin-agent`: `reasoning: true`, `thinking: undefined`
// — Cascade routes by sibling model id, not a wire param), the advisor
// descriptor MUST NOT hand the Agent a concrete `Effort.Medium` default. That
// would trip `requireSupportedEffort` inside `stream.ts` on the first prompt
// and disable the advisor session-wide with an empty
// `Supported efforts:` warning list.
//
// This mirrors the `auto`-path fix already covered by
// `auto-thinking-classifier.test.ts:145` for `clampAutoThinkingEffort`, at the
// advisor descriptor boundary.
describe("AgentSession advisor descriptor thinking level", () => {
let sharedDir: TempDir;
let authStorage: AuthStorage;
let modelRegistry: ModelRegistry;
let anthropicModel: Model;
let devinModel: Model;
beforeAll(async () => {
sharedDir = TempDir.createSync("@pi-advisor-devin-thinking-shared-");
authStorage = await AuthStorage.create(path.join(sharedDir.path(), "testauth.db"));
authStorage.setRuntimeApiKey("anthropic", "test-key");
// Seeding a runtime API key exposes the bundled Devin catalog for
// `resolveAdvisorRoleSelection` / `getAvailable()` without any live
// network discovery.
authStorage.setRuntimeApiKey("devin", "test-key");
modelRegistry = new ModelRegistry(authStorage);
const anthropic = getBundledModel("anthropic", "claude-sonnet-4-5");
const devin = getBundledModel("devin", "glm-5-2");
if (!anthropic) throw new Error("Expected bundled anthropic/claude-sonnet-4-5 to exist");
if (!devin) throw new Error("Expected bundled devin/glm-5-2 to exist");
anthropicModel = anthropic;
devinModel = devin;
});
afterAll(async () => {
authStorage.close();
try {
await sharedDir.remove();
} catch {}
});
let tempDir: TempDir;
let session: AgentSession;
let sessionManager: SessionManager;
beforeEach(async () => {
tempDir = TempDir.createSync("@pi-advisor-devin-thinking-");
sessionManager = SessionManager.create(tempDir.path(), tempDir.path());
const agent = new Agent({
initialState: {
model: anthropicModel,
systemPrompt: ["Test"],
tools: [],
messages: [],
},
});
const settings = Settings.isolated({ "compaction.enabled": false });
session = new AgentSession({
agent,
sessionManager,
settings,
modelRegistry,
advisorTools: [],
});
});
afterEach(async () => {
await session.dispose();
try {
await tempDir.remove();
} catch {}
});
it("Devin advisor with no configured thinking suffix boots without an unsupported-effort throw", () => {
// Confirm the catalog shape that triggered the bug: `reasoning: true` with
// no controllable `thinking.efforts`. If this drifts upstream the
// regression's assumptions no longer hold.
expect(devinModel.reasoning).toBe(true);
expect(devinModel.thinking).toBeUndefined();
session.settings.setModelRole("advisor", `${devinModel.provider}/${devinModel.id}`);
expect(session.setAdvisorEnabled(true)).toBe(true);
expect(session.isAdvisorActive()).toBe(true);
// Before the fix, the descriptor hardcoded `ThinkingLevel.Medium` which
// flowed to `Agent#state.thinkingLevel` and then tripped
// `requireSupportedEffort` inside `mapOptionsForApi`'s `devin-agent`
// branch on the first stream. The clamp now forwards no explicit effort
// (mirroring `clampAutoThinkingEffort`), so the Agent stores `undefined`
// and the provider's default routing applies.
const advisor = session.getAdvisorAgent();
if (!advisor) throw new Error("Expected advisor Agent to be live");
expect(advisor.state.model.provider).toBe(devinModel.provider);
expect(advisor.state.model.id).toBe(devinModel.id);
expect(advisor.state.thinkingLevel).toBeUndefined();
// `Off` is reserved for the explicit "disable reasoning" selector; the
// Devin path forwards no effort while keeping reasoning enabled.
expect(advisor.state.disableReasoning).toBe(false);
});
it("Anthropic advisor with no configured thinking suffix still gets the medium default", () => {
// Guard against over-clamping: models that support `medium` MUST keep
// receiving it so the historical advisor thinking budget is preserved.
session.settings.setModelRole("advisor", `${anthropicModel.provider}/${anthropicModel.id}`);
expect(session.setAdvisorEnabled(true)).toBe(true);
const advisor = session.getAdvisorAgent();
if (!advisor) throw new Error("Expected advisor Agent to be live");
expect(advisor.state.model.provider).toBe(anthropicModel.provider);
expect(advisor.state.thinkingLevel).toBe(Effort.Medium);
});
it("Devin advisor with an explicit :off suffix disables reasoning without clamping to inherit", () => {
// `off` is an explicit user opt-out and MUST reach the Agent as
// `disableReasoning: true` regardless of the model's effort surface. The
// clamp helper preserves `off`; verifying that here so a future change
// to the descriptor doesn't route `off` through the Devin
// no-controllable-effort fallback and silently re-enable reasoning.
session.settings.setModelRole("advisor", `${devinModel.provider}/${devinModel.id}:off`);
expect(session.setAdvisorEnabled(true)).toBe(true);
const advisor = session.getAdvisorAgent();
if (!advisor) throw new Error("Expected advisor Agent to be live");
expect(advisor.state.thinkingLevel).toBeUndefined();
expect(advisor.state.disableReasoning).toBe(true);
});
});