From 69063e7be7a03429f36f0f81adff7a297f318dd1 Mon Sep 17 00:00:00 2001 From: Zhafron Date: Mon, 31 Aug 2026 14:02:41 +0700 Subject: [PATCH 1/2] fix(historian): make temperature opt-in for reasoning models The historian defaulted temperature to 0.1 in two independent places, so every run sent a temperature the user never configured. Reasoning models reject the parameter outright: OpenAI Responses answers 400 "Unsupported parameter: temperature" and Anthropic answers 400 "`temperature` may only be set to 1 when thinking is enabled". The rejected request produces an empty assistant message, which surfaces as no_assistant and points the user at their model config even though model and endpoint are healthy. Every fallback model fails identically, because the request shape is the cause rather than the model. Removing the index.ts default alone is not enough; the destructuring default in pi-historian-runner.ts silently reapplies 0.1. The calibration extension also gated both knobs together, so an output token budget could not be applied without a temperature. Apply them independently, keeping the 32k budget working for reasoning models. All three defaults were introduced together in 10f80e58 and first released in v0.41.0. Verified by the four new calibration cases and the full pi-plugin suite (886 pass, typecheck and lint clean). --- CONFIGURATION.md | 2 +- .../historian-calibration-extension.test.ts | 52 +++++++++++++++++++ .../src/historian-calibration-extension.ts | 46 ++++++++++------ packages/pi-plugin/src/index.ts | 2 +- packages/pi-plugin/src/pi-historian-runner.ts | 2 +- 5 files changed, 84 insertions(+), 20 deletions(-) diff --git a/CONFIGURATION.md b/CONFIGURATION.md index 5bb030905..af5c70730 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -663,7 +663,7 @@ It is useful when starting a new session. It's better to choose a fast and cheap |-------|------|-------------| | `model` | `string` | Primary model. | | `fallback_models` | `string` or `string[]` | Fallback models. | -| `temperature` | `number` (0–2) | Sampling temperature. | +| `temperature` | `number` (0–2) | Sampling temperature. Omitted by default: no `temperature` is sent to the provider unless you set one. Reasoning models reject the parameter outright, so only set it for models that accept it. | | `variant` | `string` | **OpenCode only.** Agent variant — selects a thinking/reasoning preset. Pi uses `thinking_level` instead. | | `thinking_level` | `string` | **Pi only.** Explicit reasoning level (`off`/`low`/`medium`/`high`) passed to Pi for sidekick subagent runs. See `historian.thinking_level`. | | `prompt` | `string` | Persistent agent-level system prompt override. Applies to every sidekick run. | diff --git a/packages/pi-plugin/src/historian-calibration-extension.test.ts b/packages/pi-plugin/src/historian-calibration-extension.test.ts index cadbc3b87..e7bf493c6 100644 --- a/packages/pi-plugin/src/historian-calibration-extension.test.ts +++ b/packages/pi-plugin/src/historian-calibration-extension.test.ts @@ -42,4 +42,56 @@ describe("historian provider calibration", () => { ), ).toEqual({ inferenceConfig: { temperature: 0.1, maxTokens: 32_000 } }); }); + + it("never sends temperature when it is not configured", () => { + const fixtures = [ + { input: { max_tokens: 4096 }, key: "max_tokens" }, + { input: { max_completion_tokens: 4096 }, key: "max_completion_tokens" }, + { input: { max_output_tokens: 4096 }, key: "max_output_tokens" }, + { input: { maxTokens: 4096 }, key: "maxTokens" }, + ] as const; + for (const fixture of fixtures) { + const result = calibrateHistorianProviderPayload( + fixture.input, + undefined, + 32_000, + ) as Record; + expect("temperature" in result).toBe(false); + expect(result[fixture.key]).toBe(32_000); + } + }); + + it("omits temperature from nested provider shapes when unconfigured", () => { + expect( + calibrateHistorianProviderPayload( + { generationConfig: { topP: 0.9, maxOutputTokens: 4096 } }, + undefined, + 32_000, + ), + ).toEqual({ generationConfig: { topP: 0.9, maxOutputTokens: 32_000 } }); + expect( + calibrateHistorianProviderPayload( + { inferenceConfig: { maxTokens: 4096 } }, + undefined, + 32_000, + ), + ).toEqual({ inferenceConfig: { maxTokens: 32_000 } }); + }); + + it("applies temperature alone when no output budget is configured", () => { + const result = calibrateHistorianProviderPayload( + { max_tokens: 4096 }, + 0.1, + undefined, + ) as Record; + expect(result.temperature).toBe(0.1); + expect(result.max_tokens).toBe(4096); + }); + + it("returns the payload untouched when neither knob is configured", () => { + const payload = { max_tokens: 4096 }; + expect( + calibrateHistorianProviderPayload(payload, undefined, undefined), + ).toEqual({ max_tokens: 4096 }); + }); }); diff --git a/packages/pi-plugin/src/historian-calibration-extension.ts b/packages/pi-plugin/src/historian-calibration-extension.ts index 71a0e7d2b..67b362555 100644 --- a/packages/pi-plugin/src/historian-calibration-extension.ts +++ b/packages/pi-plugin/src/historian-calibration-extension.ts @@ -10,14 +10,22 @@ function finiteNumber(value: string | undefined): number | undefined { return Number.isFinite(parsed) && parsed >= 0 ? parsed : undefined; } -/** Apply the historian calibration to each provider's serialized request shape. */ +/** + * Apply the historian calibration to each provider's serialized request shape. + * + * Both knobs are optional and applied independently: reasoning models reject + * `temperature` outright, so an output-token budget must still be applicable + * on its own. + */ export function calibrateHistorianProviderPayload( payload: unknown, - temperature: number, - maxOutputTokens: number, + temperature: number | undefined, + maxOutputTokens: number | undefined, ): unknown { if (typeof payload !== "object" || payload === null || Array.isArray(payload)) return payload; + if (temperature === undefined && maxOutputTokens === undefined) + return payload; const calibrated = { ...(payload as Record) }; const generationConfig = calibrated.generationConfig; if ( @@ -27,8 +35,8 @@ export function calibrateHistorianProviderPayload( ) { calibrated.generationConfig = { ...(generationConfig as Record), - temperature, - maxOutputTokens, + ...(temperature !== undefined ? { temperature } : {}), + ...(maxOutputTokens !== undefined ? { maxOutputTokens } : {}), }; return calibrated; } @@ -40,21 +48,25 @@ export function calibrateHistorianProviderPayload( ) { calibrated.inferenceConfig = { ...(inferenceConfig as Record), - temperature, - maxTokens: maxOutputTokens, + ...(temperature !== undefined ? { temperature } : {}), + ...(maxOutputTokens !== undefined ? { maxTokens: maxOutputTokens } : {}), }; return calibrated; } - calibrated.temperature = temperature; - if ("max_output_tokens" in calibrated) { - calibrated.max_output_tokens = maxOutputTokens; - } else if ("max_completion_tokens" in calibrated) { - calibrated.max_completion_tokens = maxOutputTokens; - } else if ("max_tokens" in calibrated) { - calibrated.max_tokens = maxOutputTokens; - } else if ("maxTokens" in calibrated) { - calibrated.maxTokens = maxOutputTokens; + if (temperature !== undefined) { + calibrated.temperature = temperature; + } + if (maxOutputTokens !== undefined) { + if ("max_output_tokens" in calibrated) { + calibrated.max_output_tokens = maxOutputTokens; + } else if ("max_completion_tokens" in calibrated) { + calibrated.max_completion_tokens = maxOutputTokens; + } else if ("max_tokens" in calibrated) { + calibrated.max_tokens = maxOutputTokens; + } else if ("maxTokens" in calibrated) { + calibrated.maxTokens = maxOutputTokens; + } } return calibrated; } @@ -64,7 +76,7 @@ export default function historianCalibrationExtension(pi: ExtensionAPI): void { const maxOutputTokens = finiteNumber( process.env[HISTORIAN_MAX_OUTPUT_TOKENS_ENV], ); - if (temperature === undefined || maxOutputTokens === undefined) return; + if (temperature === undefined && maxOutputTokens === undefined) return; pi.on("before_provider_request", (event) => calibrateHistorianProviderPayload( event.payload, diff --git a/packages/pi-plugin/src/index.ts b/packages/pi-plugin/src/index.ts index 846e45e5e..e396fd379 100644 --- a/packages/pi-plugin/src/index.ts +++ b/packages/pi-plugin/src/index.ts @@ -703,7 +703,7 @@ export function resolveHistorianFromConfig( fallbackModels, historianChunkTokens, timeoutMs: config.historian_timeout_ms, - temperature: historian?.temperature ?? 0.1, + temperature: historian?.temperature, maxOutputTokens: historian?.maxTokens ?? 32_000, // `historian.two_pass` runs an editor pass after a successful // first pass to clean low-signal U: lines and cross-compartment diff --git a/packages/pi-plugin/src/pi-historian-runner.ts b/packages/pi-plugin/src/pi-historian-runner.ts index 465caea22..760279dbc 100644 --- a/packages/pi-plugin/src/pi-historian-runner.ts +++ b/packages/pi-plugin/src/pi-historian-runner.ts @@ -444,7 +444,7 @@ export async function runPiHistorian(deps: PiHistorianDeps): Promise { refreshBoundarySnapshot, currentContextLimit, historianTimeoutMs = DEFAULT_HISTORIAN_TIMEOUT_MS, - temperature = 0.1, + temperature, maxOutputTokens = 32_000, signal, retryBackoffMs, From 55ad3d123e047b47895da0842401a027ddb9d1ed Mon Sep 17 00:00:00 2001 From: Zhafron Date: Mon, 31 Aug 2026 14:02:41 +0700 Subject: [PATCH 2/2] fix(subagent): surface provider error behind empty assistant text The empty-assistant-text branch is evaluated before the stopReason error branch, so a provider rejection whose message carries no text always settles as no_assistant and the captured finalErrorMessage holding the real HTTP error is discarded. The user only ever sees "pi assistant produced empty text", which hides the actual cause and sends debugging toward the model configuration. Append the provider error to the failure message when one was captured, leaving the reason code and retry semantics unchanged. Proven with a negative control: reverting only this change fails exactly the new regression test (86 pass, 1 fail) and passes with it (87 pass). --- .../pi-plugin/src/subagent-runner.test.ts | 28 +++++++++++++++++++ packages/pi-plugin/src/subagent-runner.ts | 11 +++++--- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/packages/pi-plugin/src/subagent-runner.test.ts b/packages/pi-plugin/src/subagent-runner.test.ts index b02d91a6b..660534d69 100644 --- a/packages/pi-plugin/src/subagent-runner.test.ts +++ b/packages/pi-plugin/src/subagent-runner.test.ts @@ -1558,6 +1558,34 @@ describe("PiSubagentRunner spawn lifecycle", () => { }); }); + it("surfaces the provider error behind empty assistant text", async () => { + const child = createMockChild(); + const { runner } = runnerWith(child); + + const resultPromise = runner.run(baseOptions); + child.writeStdoutLine( + agentEnd([ + { + role: "assistant", + content: [], + stopReason: "error", + errorMessage: + "OpenAI API error (400): Unsupported parameter: temperature", + }, + ]), + ); + child.emitClose(0); + + expect(await resultPromise).toEqual({ + ok: false, + reason: "no_assistant", + error: + "pi assistant produced empty text (provider error: OpenAI API error (400): Unsupported parameter: temperature)", + durationMs: expect.any(Number), + meta: { stderr: undefined, sawProtocolOutput: true }, + }); + }); + it("returns no_assistant for empty stdout and successful exit", async () => { // Issue #238: an empty-stdout exit-0 primary now fires the one-shot // isolated retry. When the isolated attempt ALSO exits 0 with no output, diff --git a/packages/pi-plugin/src/subagent-runner.ts b/packages/pi-plugin/src/subagent-runner.ts index bfeea7f74..d0a88fd23 100644 --- a/packages/pi-plugin/src/subagent-runner.ts +++ b/packages/pi-plugin/src/subagent-runner.ts @@ -1378,13 +1378,16 @@ export class PiSubagentRunner implements SubagentRunner { trimmedAssistantText === null || trimmedAssistantText.length === 0 ) { + const emptyAssistantReason = + trimmedAssistantText === null + ? "pi agent_end did not include an assistant message" + : "pi assistant produced empty text"; settle({ ok: false, reason: "no_assistant", - error: - trimmedAssistantText === null - ? "pi agent_end did not include an assistant message" - : "pi assistant produced empty text", + error: finalErrorMessage + ? `${emptyAssistantReason} (provider error: ${finalErrorMessage})` + : emptyAssistantReason, durationMs: Date.now() - startTime, // Pi machinery worked (agent_end / terminal message_end seen); // the model just returned empty text. Mark protocol output as