-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(zai): persist the Responses destination the router already applies #4321
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
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 |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { getProviderRegistryEntry, providerMatchesRegistryTransport } from "./registry"; | ||
| import type { OcxConfig } from "../types"; | ||
|
|
||
| export const ZAI_PROVIDER_ID = "zai"; | ||
| export const ZAI_RESPONSES_DEFAULT_VERSION = 1; | ||
|
|
||
| /** | ||
| * Persist the Responses destination the router already applies to the `zai` row. | ||
| * | ||
| * The Z.AI coding plan moved from Chat Completions at /api/coding/paas/v4 to Responses at | ||
| * /api/v1 (#4297). A config written before that move still stores the Chat adapter and the old | ||
| * base URL, and `routedProviderConfig()` rewrites both on every request because the registry | ||
| * entry owns a fixed destination. The row therefore already talks Responses while the dashboard, | ||
| * `ocx doctor` and any direct config reader show the retired Chat endpoint, and each boot logs a | ||
| * "configured baseUrl is ignored" warning about a value the user never chose. | ||
| * | ||
| * This migration writes the canonical pair once so the stored row matches the live wire. It is | ||
| * behavior-preserving by construction: it only rewrites rows the router canonicalizes anyway. | ||
| * Chat remains reachable per model through `modelAdapters`, and the persisted marker keeps a | ||
| * later explicit Chat choice from being migrated again. | ||
| * | ||
| * A custom-named provider pointing at the retired endpoint is deliberately left alone. The router | ||
| * does not canonicalize it, so rewriting it would change a wire the operator actually configured; | ||
| * `destinationAliases` already gives it this row's metadata. | ||
| */ | ||
| export function migrateZaiResponsesDefault(config: OcxConfig): boolean { | ||
| const provider = config.providers[ZAI_PROVIDER_ID]; | ||
| if (!provider || (provider.zaiResponsesDefaultVersion ?? 0) >= ZAI_RESPONSES_DEFAULT_VERSION) return false; | ||
| const entry = getProviderRegistryEntry(ZAI_PROVIDER_ID); | ||
| if (!entry) return false; | ||
| // Fail closed if a later registry edit makes this destination operator-owned: only a fixed, | ||
| // non-templated endpoint is canonicalized at request time, so only that one may be persisted. | ||
| if (entry.allowBaseUrlOverride || /\{[^}]*\}/.test(entry.baseUrl)) return false; | ||
| if (!providerMatchesRegistryTransport(ZAI_PROVIDER_ID, provider)) return false; | ||
|
Contributor
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 | 🟠 Major | 🏗️ Heavy lift Use a legacy-source predicate before the rewrite. Line 34 rejects every row that this migration must upgrade. Match the explicit retired adapter and endpoint as the migration source. Keep lines 29-33 as the destination safety check. This makes the migration update only the documented legacy 🤖 Prompt for AI Agents |
||
| config.providers = { | ||
| ...config.providers, | ||
| [ZAI_PROVIDER_ID]: { | ||
| ...provider, | ||
| adapter: entry.adapter, | ||
| baseUrl: entry.baseUrl, | ||
| zaiResponsesDefaultVersion: ZAI_RESPONSES_DEFAULT_VERSION, | ||
|
Comment on lines
+39
to
+41
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.
A pre-#4307 Z.AI row normally has neither of the newly introduced path fields, but this migration writes only the adapter and base URL. Its persisted representation therefore implies the Responses adapter fallback Useful? React with 👍 / 👎. |
||
| }, | ||
| }; | ||
| return true; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ import { grokDefaultReasoningEffort } from "../grok/effort"; | |
| import { flushConfigDirHardening } from "../config/paths"; | ||
| import { migrateStartupSubagentModels } from "./subagent-models-startup"; | ||
| import { migrateStartupXaiResponses } from "./xai-responses-startup"; | ||
| import { migrateStartupZaiResponses } from "./zai-responses-startup"; | ||
| import { reconcileOAuthProviders } from "../oauth"; | ||
| import { withCatalogWriteSerialization } from "../codex/catalog-write-serialization"; | ||
| import { invalidateCodexModelsCacheWithPermit } from "../codex/catalog/sync"; | ||
|
|
@@ -666,7 +667,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server<W | |
| // Reconcile disk-backed presets first: it replaces provider rows and must not undo | ||
| // an in-memory wire upgrade when that upgrade's persistence is temporarily unavailable. | ||
| reconcileOAuthProviders(startupConfig); | ||
| const config = migrateStartupXaiResponses(startupConfig); | ||
| const config = migrateStartupZaiResponses(migrateStartupXaiResponses(startupConfig)); | ||
|
Contributor
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. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Combine the X.AI and Z.AI startup migrations into one persistence mutation. At Apply both rewrites to the same 🤖 Prompt for AI Agents |
||
| warnAgentTaskRecoveryStartup(config); | ||
| setLiveStateStoreConfig(config); | ||
| applyProxyEnv(config); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,7 @@ import { | |
| XAI_RESPONSES_DEFAULT_VERSION, | ||
| xaiResponsesOptInState, | ||
| } from "../../providers/xai-responses-opt-in"; | ||
| import { ZAI_PROVIDER_ID } from "../../providers/zai-responses-migration"; | ||
| import { dropProviderCustomModels } from "../../providers/provider-id-rewrite"; | ||
|
|
||
| import { isPlainRecord, parseDebugLogQuery, tokPerSecondResult, unavailableCostReason, costResult, requestLogDto, stripRegistryOnlyStaticHeaders, fetchAllModels } from "./shared"; | ||
|
|
@@ -1082,6 +1083,14 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise<Resp | |
| prov.xaiResponsesDefaultVersion = latest.xaiResponsesDefaultVersion; | ||
| } | ||
| } | ||
| // Same reason for the Z.AI marker: the provider form never carries it, and losing it on an | ||
| // unrelated edit would let the one-time wire rewrite run a second time. | ||
| if (name === ZAI_PROVIDER_ID) { | ||
| const latest = config.providers[name]; | ||
| if (latest?.zaiResponsesDefaultVersion !== undefined) { | ||
| prov.zaiResponsesDefaultVersion = latest.zaiResponsesDefaultVersion; | ||
| } | ||
|
Comment on lines
+1088
to
+1092
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.
The new tests call the migration helpers directly but never exercise this AGENTS.md reference: src/AGENTS.md:L24-L24 Useful? React with 👍 / 👎. |
||
| } | ||
| // Reapply pins to the latest live row after DNS/import awaits, then validate the | ||
| // complete draft before adopting any provider/default state. | ||
| const latest = config.providers[name]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { mutatePersistedConfig } from "../config"; | ||
| import { migrateZaiResponsesDefault } from "../providers/zai-responses-migration"; | ||
| import type { OcxConfig } from "../types"; | ||
|
|
||
| /** Rebase the one-time Z.AI wire upgrade before initializing any live config consumers. */ | ||
| export function migrateStartupZaiResponses(config: OcxConfig): OcxConfig { | ||
| const projection = { ...config }; | ||
| if (!migrateZaiResponsesDefault(projection)) return config; | ||
| try { | ||
| const outcome = mutatePersistedConfig(fresh => ({ | ||
| changed: migrateZaiResponsesDefault(fresh), | ||
| value: fresh, | ||
| })); | ||
| if (outcome.status !== "unavailable") return outcome.value; | ||
| console.warn(`[zai-responses-migration] Persistence unavailable (${outcome.reason}); using Responses in memory only.`); | ||
| } catch { | ||
| // Filesystem errors can carry private paths. Startup must still remain available. | ||
| console.warn("[zai-responses-migration] Persistence failed; using Responses in memory only."); | ||
| } | ||
| return projection; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,8 @@ import { DEFAULT_SUBAGENT_MODELS, migrateSubagentModels } from "../../src/config | |
| import { migrateStartupSubagentModels } from "../../src/server/subagent-models-startup"; | ||
| import { migrateXaiResponsesDefault } from "../../src/providers/xai-responses-opt-in"; | ||
| import { migrateStartupXaiResponses } from "../../src/server/xai-responses-startup"; | ||
| import { migrateZaiResponsesDefault } from "../../src/providers/zai-responses-migration"; | ||
| import { migrateStartupZaiResponses } from "../../src/server/zai-responses-startup"; | ||
| import * as configStore from "../../src/config"; | ||
| import { runClaudeAuthModeMigration } from "../../src/claude/auth-mode-migration"; | ||
| import { providerManagementConfigError } from "../../src/server/auth-cors"; | ||
|
|
@@ -303,6 +305,64 @@ describe("one-time Grok Responses upgrade", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("one-time Z.AI Responses upgrade", () => { | ||
| const CANONICAL = { adapter: "openai-responses", baseUrl: "https://api.z.ai" }; | ||
|
Contributor
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Include the canonical Responses path in the migration and its expected result.
The migrated row will therefore persist 🤖 Prompt for AI AgentsSources: Coding guidelines, Path instructions |
||
| const RETIRED = { adapter: "openai-chat", baseUrl: "https://api.z.ai/api/coding/paas/v4" }; | ||
|
|
||
| function legacy() { | ||
| return { | ||
| ...getDefaultConfig(), | ||
| providers: { | ||
| zai: { ...RETIRED, authMode: "key" as const, defaultModel: "glm-5.3" }, | ||
| }, | ||
| defaultProvider: "zai", | ||
| }; | ||
| } | ||
|
|
||
| test("read-only load keeps the retired endpoint; startup persists the canonical wire once", () => { | ||
| saveConfig(legacy()); | ||
| const before = readFileSync(getConfigPath(), "utf8"); | ||
| const config = loadConfig(); | ||
| expect(config.providers.zai).toMatchObject(RETIRED); | ||
| expect(readFileSync(getConfigPath(), "utf8")).toBe(before); | ||
|
|
||
| const upgraded = migrateStartupZaiResponses(config); | ||
| expect(upgraded.providers.zai).toMatchObject({ ...CANONICAL, zaiResponsesDefaultVersion: 1 }); | ||
| expect(upgraded.providers.zai!.defaultModel).toBe("glm-5.3"); | ||
| expect(loadConfig().providers.zai).toEqual(upgraded.providers.zai); | ||
| // The caller's snapshot is not mutated in place, and a second boot is a no-op. | ||
| expect(config.providers.zai).toMatchObject(RETIRED); | ||
| expect(migrateZaiResponsesDefault(upgraded)).toBe(false); | ||
| }); | ||
|
|
||
| test.each([1, 2])("an existing marker of version %i blocks a second rewrite", version => { | ||
| const config = legacy(); | ||
| config.providers.zai.zaiResponsesDefaultVersion = version; | ||
| saveConfig(config); | ||
| expect(migrateStartupZaiResponses(loadConfig()).providers.zai).toEqual(config.providers.zai); | ||
| expect(loadConfig().providers.zai!.zaiResponsesDefaultVersion).toBe(version); | ||
| }); | ||
|
|
||
| test("a custom-named row at the retired endpoint keeps its configured wire", () => { | ||
| const source = legacy(); | ||
| const custom = { ...source, defaultProvider: "my-zai", providers: { "my-zai": source.providers.zai } }; | ||
| const before = structuredClone(custom); | ||
| expect(migrateZaiResponsesDefault(custom)).toBe(false); | ||
| expect(custom).toEqual(before); | ||
| }); | ||
|
|
||
| test("unavailable persistence preserves disk and returns an isolated projection", () => { | ||
| const config = legacy(); | ||
| writeConfig("{ invalid"); | ||
| const warn = spyOn(console, "warn").mockImplementation(() => {}); | ||
| try { | ||
| expect(migrateStartupZaiResponses(config).providers.zai).toMatchObject(CANONICAL); | ||
| expect(readFileSync(getConfigPath(), "utf8")).toBe("{ invalid"); | ||
| expect(config.providers.zai).toMatchObject(RETIRED); | ||
| } finally { warn.mockRestore(); } | ||
| }); | ||
| }); | ||
|
|
||
| function writeConfig(content: unknown): void { | ||
| writeFileSync( | ||
| getConfigPath(), | ||
|
|
||
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.
This adds a new
src/providers/migration and changessrc/config.tsandsrc/server/, but the commit updates onlystructure/transports/responses.md.structure/INDEX.mdmaps these source areas to several additional documents—for example,src/providers/toruntime.md,subagents.md,transports/inventory.md, andproviders/xai-grok.md, andsrc/config.tsto four other documents. Update every mapped document in this change, or correct the manifest ownership if those documents do not describe these areas.AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.