-
Notifications
You must be signed in to change notification settings - Fork 21
fix(pi): recover custom proxy context overflows #293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import type { | |
| ExtensionCommandContextActions, | ||
| ToolDefinition, | ||
| } from "@earendil-works/pi-coding-agent"; | ||
| import type { AssistantMessageEvent, Context, Model, SimpleStreamOptions } from "@earendil-works/pi-ai"; | ||
| import { createJiti } from "jiti/static"; | ||
| import type { AgentConfig, AgentModelConfig, AgentToolDefinition, JsonObject } from "nemo-fabric-adapter-contract"; | ||
| import { LifecycleError, type AdapterStartInput } from "nemo-fabric-adapters-common"; | ||
|
|
@@ -30,6 +31,44 @@ interface PiToolFactoryContext { | |
| workspace: string; | ||
| } | ||
|
|
||
| export function withCustomBaseUrl<T extends { api: string; baseUrl: string; compat?: object }>( | ||
| catalogModel: T, | ||
| baseUrl: string | null | undefined, | ||
| ): T { | ||
| if (!baseUrl) { | ||
| return catalogModel; | ||
| } | ||
| if (catalogModel.api !== "openai-completions") { | ||
| return { ...catalogModel, baseUrl }; | ||
| } | ||
| return { | ||
| ...catalogModel, | ||
| baseUrl, | ||
| // Generic OpenAI-compatible proxies may reject provider-specific | ||
| // reasoning_content fields when Pi replays an assistant tool call. | ||
| compat: { ...catalogModel.compat, requiresThinkingAsText: true }, | ||
| }; | ||
| } | ||
|
|
||
| export function modelAwareCompactionReserveTokens( | ||
| configuredReserveTokens: number, | ||
| maxOutputTokens: number, | ||
| ): number { | ||
| return Math.max(configuredReserveTokens, maxOutputTokens); | ||
| } | ||
|
|
||
| const OPAQUE_PROXY_SERVER_ERROR = /^500 status code \(no body\)$/iu; | ||
|
|
||
| export function classifyOpaqueProxyContextOverflow( | ||
| errorMessage: string | undefined, | ||
| contextWindow: number, | ||
| ): string | undefined { | ||
| if (errorMessage === undefined || contextWindow <= 0 || !OPAQUE_PROXY_SERVER_ERROR.test(errorMessage)) { | ||
| return errorMessage; | ||
| } | ||
| return `maximum context length is ${contextWindow} tokens (${errorMessage} from the configured model proxy)`; | ||
| } | ||
|
|
||
| type PiToolFactory = (context: PiToolFactoryContext) => ToolDefinition | Promise<ToolDefinition>; | ||
|
|
||
| const PI_BUILTIN_TOOL_NAMES = new Set(["read", "bash", "edit", "write", "grep", "find", "ls"]); | ||
|
|
@@ -39,6 +78,7 @@ const PI_HARNESS_INSTALL_COMMAND = | |
|
|
||
| interface PiSdkModules { | ||
| InMemoryCredentialStore: typeof import("@earendil-works/pi-ai").InMemoryCredentialStore; | ||
| createAssistantMessageEventStream: typeof import("@earendil-works/pi-ai").createAssistantMessageEventStream; | ||
| createAgentSession: typeof import("@earendil-works/pi-coding-agent").createAgentSession; | ||
| DefaultResourceLoader: typeof import("@earendil-works/pi-coding-agent").DefaultResourceLoader; | ||
| ModelRuntime: typeof import("@earendil-works/pi-coding-agent").ModelRuntime; | ||
|
|
@@ -75,6 +115,7 @@ async function loadPiSdk(): Promise<PiSdkModules> { | |
|
|
||
| if ( | ||
| typeof ai.InMemoryCredentialStore !== "function" || | ||
| typeof ai.createAssistantMessageEventStream !== "function" || | ||
| typeof codingAgent.createAgentSession !== "function" || | ||
| typeof codingAgent.DefaultResourceLoader !== "function" || | ||
| typeof codingAgent.ModelRuntime !== "function" || | ||
|
|
@@ -89,6 +130,7 @@ async function loadPiSdk(): Promise<PiSdkModules> { | |
|
|
||
| return { | ||
| InMemoryCredentialStore: ai.InMemoryCredentialStore, | ||
| createAssistantMessageEventStream: ai.createAssistantMessageEventStream, | ||
| createAgentSession: codingAgent.createAgentSession, | ||
| DefaultResourceLoader: codingAgent.DefaultResourceLoader, | ||
| ModelRuntime: codingAgent.ModelRuntime, | ||
|
|
@@ -97,6 +139,64 @@ async function loadPiSdk(): Promise<PiSdkModules> { | |
| }; | ||
| } | ||
|
|
||
| function streamFailure(model: Model<any>, error: unknown): AssistantMessageEvent { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| return { | ||
| type: "error", | ||
| reason: "error", | ||
| error: { | ||
| role: "assistant", | ||
| content: [], | ||
| api: model.api, | ||
| provider: model.provider, | ||
| model: model.id, | ||
| usage: { | ||
| input: 0, | ||
| output: 0, | ||
| cacheRead: 0, | ||
| cacheWrite: 0, | ||
| totalTokens: 0, | ||
| cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, | ||
| }, | ||
| stopReason: "error", | ||
| errorMessage, | ||
| timestamp: Date.now(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| function installOpaqueProxyOverflowRecovery(session: AgentSession, pi: PiSdkModules): void { | ||
| const originalStream = session.agent.streamFunction.bind(session.agent); | ||
| session.agent.streamFunction = async ( | ||
| model: Model<any>, | ||
| context: Context, | ||
| options?: SimpleStreamOptions, | ||
| ) => { | ||
| const source = await originalStream(model, context, options); | ||
| const target = pi.createAssistantMessageEventStream(); | ||
| void (async () => { | ||
| try { | ||
| for await (const event of source) { | ||
| if (event.type !== "error") { | ||
| target.push(event); | ||
| continue; | ||
| } | ||
| const errorMessage = classifyOpaqueProxyContextOverflow( | ||
| event.error.errorMessage, | ||
| model.contextWindow, | ||
| ); | ||
| target.push({ ...event, error: { ...event.error, errorMessage } }); | ||
| } | ||
| target.end(); | ||
| } catch (error) { | ||
| target.push(streamFailure(model, error)); | ||
| target.end(); | ||
| } | ||
| })(); | ||
| return target; | ||
| }; | ||
| } | ||
|
|
||
| function selectModel(config: AgentConfig): AgentModelConfig { | ||
| const entries = Object.entries(config.models ?? {}); | ||
| if (entries.length === 0) { | ||
|
|
@@ -491,7 +591,15 @@ export class PiSdkSessionFactory implements PiSessionFactory { | |
| if (catalogModel === undefined) { | ||
| throw new LifecycleError("pi_model_unknown", "The selected provider and model are not present in Pi's catalog"); | ||
| } | ||
| const model = selected.base_url ? { ...catalogModel, baseUrl: selected.base_url } : catalogModel; | ||
| const model = withCustomBaseUrl(catalogModel, selected.base_url); | ||
| settings.applyOverrides({ | ||
| compaction: { | ||
| reserveTokens: modelAwareCompactionReserveTokens( | ||
| settings.getCompactionReserveTokens(), | ||
| model.maxTokens, | ||
| ), | ||
| }, | ||
| }); | ||
| const enabled = input.config.tools?.enabled; | ||
| const blocked = input.config.tools?.blocked ?? []; | ||
| const state = { shutdownRequested: false }; | ||
|
|
@@ -507,6 +615,9 @@ export class PiSdkSessionFactory implements PiSessionFactory { | |
| tools: enabled === null ? undefined : enabled, | ||
| excludeTools: blocked, | ||
| }); | ||
| if (selected.base_url !== undefined && selected.base_url !== null) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Use the same custom URL predicate for recovery installation. When 🤖 Prompt for AI Agents |
||
| installOpaqueProxyOverflowRecovery(session, pi); | ||
| } | ||
| const handle = new PiSdkSessionHandle(session, state); | ||
| try { | ||
| const blockedNames = new Set(blocked); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,45 @@ import { tmpdir } from "node:os"; | |
| import { join } from "node:path"; | ||
| import test from "node:test"; | ||
|
|
||
| import { PiSdkSessionFactory, resolveCustomTools } from "../dist/pi-sdk.js"; | ||
| import { | ||
| classifyOpaqueProxyContextOverflow, | ||
| modelAwareCompactionReserveTokens, | ||
| PiSdkSessionFactory, | ||
| resolveCustomTools, | ||
| withCustomBaseUrl, | ||
| } from "../dist/pi-sdk.js"; | ||
|
|
||
| test("uses standard content when replaying reasoning through a custom model proxy", () => { | ||
| const catalogModel = { | ||
| api: "openai-completions", | ||
| baseUrl: "https://integrate.api.nvidia.com/v1", | ||
| compat: { supportsStore: false }, | ||
| }; | ||
|
|
||
| assert.deepEqual(withCustomBaseUrl(catalogModel, "http://model-proxy:10240"), { | ||
| api: "openai-completions", | ||
| baseUrl: "http://model-proxy:10240", | ||
| compat: { supportsStore: false, requiresThinkingAsText: true }, | ||
| }); | ||
| assert.strictEqual(withCustomBaseUrl(catalogModel, undefined), catalogModel); | ||
| }); | ||
|
|
||
| test("reserves enough context for the selected model's maximum output", () => { | ||
| assert.equal(modelAwareCompactionReserveTokens(16_384, 65_536), 65_536); | ||
| assert.equal(modelAwareCompactionReserveTokens(65_536, 32_768), 65_536); | ||
| }); | ||
|
|
||
| test("classifies only an exact opaque custom-proxy error as context overflow", () => { | ||
| assert.match( | ||
| classifyOpaqueProxyContextOverflow("500 status code (no body)", 262_144), | ||
| /maximum context length is 262144 tokens/u, | ||
| ); | ||
| assert.equal( | ||
| classifyOpaqueProxyContextOverflow("503 status code (no body)", 262_144), | ||
| "503 status code (no body)", | ||
| ); | ||
| assert.equal(classifyOpaqueProxyContextOverflow("500 status code (no body)", 0), "500 status code (no body)"); | ||
| }); | ||
|
Comment on lines
+10
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add a stream-wrapper regression test. The current test calls 🤖 Prompt for AI Agents |
||
|
|
||
| test("rejects append system instructions before loading the Pi harness", async () => { | ||
| const factory = new PiSdkSessionFactory(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: NVIDIA/NeMo-Fabric
Length of output: 7814
🏁 Script executed:
Repository: NVIDIA/NeMo-Fabric
Length of output: 1900
Record the documentation validation limitation
The changed
adapters/typescript/pi/README.mdrequiresjust docswhen practical. Thejustexecutable is unavailable in this environment, so record that the check was not run for this reason.🤖 Prompt for AI Agents
Source: Coding guidelines