From 65822b008740af75cd2568595cb60602de80d899 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Sat, 15 Aug 2026 23:15:00 -0700 Subject: [PATCH 1/3] fix(config): drop only the invalid routing profile, not the whole config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single `routingProfiles` candidate naming a disabled provider failed the document-level parse, so the loader fell back to the built-in default. The operator's eleven providers, every API key and the entire `modelCosts` table disappeared while the proxy carried on serving and reporting healthy — the only visible symptom being a `404 unknown_profile` from a dry-run, which reads like a broken profile rather than a discarded config. `routingProfileIssues()` and `comboConfigIssues()` are both evaluated per entry but reported through `ctx.addIssue` in the schema's `superRefine`, which makes every entry-level finding a document-level failure. The merge-defaults retry cannot help: the offending entry is still there. So when a parse failure is confined to those two sections, drop exactly the entries it blamed and re-parse. Everything outside them survives. If any issue falls outside — or names the container rather than an entry — nothing is salvaged and the existing backup-and-defaults path runs unchanged. The whole entry goes rather than the individual candidate. A profile that quietly loses one candidate still routes, just not where it was told to, and a policy that silently changed shape is worse than one that is plainly absent. Absent is also the loud option: a dry-run against it answers `unknown_profile`, which together with the new warning points at the real mistake. This follows the salvage rule already established for `apiKeys` in this schema — "a key the user still has deployed must not be collateral damage for one bad neighbour". Validation is unchanged; only the blast radius is. The warning names the dropped entries, the offending provider and the reason, and no `config.json.invalid-*` backup is written, since nothing was discarded — the reporter had accumulated ten of those before noticing. --- src/config.ts | 95 +++++++++++++++++++++++++++++++++++ tests/config.test.ts | 115 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+) diff --git a/src/config.ts b/src/config.ts index 4b83c82954..02f9a9386d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2172,6 +2172,27 @@ export function loadConfig(): OcxConfig { warnDegradedAgentTaskRecovery(parsed); return withRefreshedCostOverlays(normalizeClaudeSubagentEffort(normalizeNativeSubagentSync(config, parsed), parsed)); } + // Still failing, but if every complaint is about one or more named entries + // in an independent section, drop exactly those and keep the rest. Falling + // back to defaults here would silently retire the operator's providers, + // keys and prices over a mistake in one routing profile. + const salvaged = dropInvalidConfigSections(merged, retryResult.error); + if (salvaged) { + const salvagedResult = configSchema.safeParse(salvaged.candidate); + if (salvagedResult.success) { + warnDroppedConfigSections(configPath, salvaged.dropped, retryResult.error); + const config = normalizeApiKeyIds(salvagedResult.data as OcxConfig); + warnDegradedHostname(parsed, config); + warnDegradedApiKeys(parsed, config); + warnDegradedCodexAccountPriorities(parsed, config); + warnDegradedClaudeSubagentEffort(parsed); + warnDegradedNativeSubagentConfig(parsed, config); + warnDegradedCodexAccountPicker(parsed); + warnDegradedUpstreamHostCircuitThreshold(parsed); + warnDegradedAgentTaskRecovery(parsed); + return withRefreshedCostOverlays(normalizeClaudeSubagentEffort(normalizeNativeSubagentSync(config, parsed), parsed)); + } + } // Merge couldn't fix it — truly broken config warnAndBackupInvalidConfig(configPath, result.error); return getDefaultConfig(); @@ -3428,6 +3449,80 @@ function warnConfigRepaired(configPath: string, error: z.ZodError): void { console.error(`opencodex config at ${configPath}: repaired missing field(s) [${fields}] with defaults. Your providers and accounts are preserved.`); } +/** + * Sections whose entries are independent of one another, so one bad entry is + * safe to drop without changing what the rest mean. + * + * Both are validated entry-by-entry in the `superRefine` above, which raises + * every finding as a *document*-level issue. That is what made a single routing + * candidate naming a disabled provider discard the operator's whole config — + * all eleven providers, every API key, and the entire `modelCosts` table — + * while the proxy carried on serving from built-in defaults and reporting + * healthy. + */ +const SALVAGEABLE_CONFIG_SECTIONS = ["routingProfiles", "combos"] as const; + +/** + * Drop just the named entries a parse failure blamed, so the rest of the + * document survives. + * + * Returns `null` when the failure was not confined to those sections — the + * caller then keeps its existing behaviour rather than guessing. + * + * The whole entry goes, not the individual offending candidate. A routing + * profile that quietly loses one candidate still routes, just not where the + * operator said it should, and a policy that silently changed shape is a worse + * outcome than one that is plainly absent. Absent is also the loud option: a + * dry-run against it answers `unknown_profile`, which — paired with the warning + * this emits — points at the real mistake. + */ +function dropInvalidConfigSections( + parsed: unknown, + error: z.ZodError, +): { candidate: Record; dropped: string[] } | null { + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return null; + + const doomed = new Map>(); + for (const issue of error.issues) { + const [section, id] = issue.path; + if (typeof section !== "string" || typeof id !== "string") return null; + if (!(SALVAGEABLE_CONFIG_SECTIONS as readonly string[]).includes(section)) return null; + // A complaint about the container itself ("combos must be an object") is + // not about one entry, so there is nothing selective to drop. + if (issue.path.length < 2) return null; + let ids = doomed.get(section); + if (!ids) doomed.set(section, ids = new Set()); + ids.add(id); + } + if (doomed.size === 0) return null; + + const candidate: Record = { ...(parsed as Record) }; + const dropped: string[] = []; + for (const [section, ids] of doomed) { + const current = candidate[section]; + if (!current || typeof current !== "object" || Array.isArray(current)) return null; + const kept: Record = {}; + for (const [key, value] of Object.entries(current as Record)) { + if (ids.has(key)) dropped.push(`${section}.${key}`); + else kept[key] = value; + } + candidate[section] = kept; + } + return dropped.length > 0 ? { candidate, dropped } : null; +} + +function warnDroppedConfigSections(configPath: string, dropped: string[], error: z.ZodError): void { + if (warnedConfigFallbacks.has(configPath)) return; + warnedConfigFallbacks.add(configPath); + const reasons = error.issues + .map(issue => `${issue.path.join(".")}: ${issue.message}`) + .join("; "); + console.error( + `opencodex config at ${configPath}: dropped [${dropped.join(", ")}] and loaded the rest — ${reasons}. ` + + "Everything else in your config, including providers and modelCosts, is preserved.", + ); +} + export function readPidFileValue(): number | null { try { return parsePidFile(readFileSync(getPidPath(), "utf-8")); diff --git a/tests/config.test.ts b/tests/config.test.ts index 5ec0d918a7..009acb08b0 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1580,6 +1580,121 @@ describe("opencodex config defaults", () => { } }); + describe("one bad entry in an independent section does not discard the config (#1785)", () => { + /** Two usable providers, a disabled one, prices, and a profile worth keeping. */ + function configWith(extra: Record): void { + writeConfig({ + port: 10100, + providers: { + TR: { adapter: "openai-chat", baseUrl: "https://tr.example/v1", disabled: true }, + keep1: { adapter: "openai-chat", baseUrl: "https://keep1.example/v1" }, + keep2: { adapter: "openai-chat", baseUrl: "https://keep2.example/v1" }, + }, + modelCosts: { "keep1/m": { input: 1, output: 2 } }, + ...extra, + }); + } + + test("a candidate naming a disabled provider drops only its profile", () => { + configWith({ + routingProfiles: { + good: { candidates: [{ provider: "keep1", model: "m" }] }, + bad: { candidates: [{ provider: "TR", model: "moonshotai/kimi-k3" }] }, + }, + }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const loaded = loadConfig() as Record; + + // The reported symptom was 11 providers on disk and 1 served. + expect(Object.keys(loaded.providers)).toEqual(expect.arrayContaining(["TR", "keep1", "keep2"])); + expect(loaded.modelCosts).toHaveProperty("keep1/m"); + expect(Object.keys(loaded.routingProfiles)).toEqual(["good"]); + + // Naming both the profile and why, so the operator can act on it. + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("routingProfiles.bad")); + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("is disabled")); + } finally { + errorSpy.mockRestore(); + } + }); + + test("salvaging is not a fallback, so no invalid-* backup piles up", () => { + // The reporter accumulated 10 of these before noticing anything was wrong; + // a backup on every load is the signal that the config was discarded. + configWith({ routingProfiles: { bad: { candidates: [{ provider: "TR", model: "m" }] } } }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + loadConfig(); + expect(backupNames()).toEqual([]); + } finally { + errorSpy.mockRestore(); + } + }); + + test("combos are salvaged the same way", () => { + configWith({ + combos: { + good: { members: [{ provider: "keep1", model: "m" }] }, + bad: { members: [{ provider: "nope-not-configured", model: "m" }] }, + }, + }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const loaded = loadConfig() as Record; + expect(Object.keys(loaded.providers)).toEqual(expect.arrayContaining(["keep1", "keep2"])); + expect(Object.keys(loaded.combos ?? {})).not.toContain("bad"); + } finally { + errorSpy.mockRestore(); + } + }); + + test("every bad profile is dropped, not just the first", () => { + configWith({ + routingProfiles: { + bad1: { candidates: [{ provider: "TR", model: "m" }] }, + good: { candidates: [{ provider: "keep1", model: "m" }] }, + bad2: { candidates: [{ provider: "also-not-configured", model: "m" }] }, + }, + }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + expect(Object.keys((loadConfig() as Record).routingProfiles)).toEqual(["good"]); + } finally { + errorSpy.mockRestore(); + } + }); + + test("a failure outside those sections still falls back, unchanged", () => { + // The salvage path must not become a way to load configs that are broken + // somewhere it cannot reason about. + writeConfig({ + port: 10100, + providers: { + custom: { adapter: "openai-chat", baseUrl: "https://example.test/v1", headers: { Authorization: "Bearer secret" } }, + }, + routingProfiles: { bad: { candidates: [{ provider: "nope", model: "m" }] } }, + }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + expect(loadConfig()).toEqual(getDefaultConfig()); + expect(backupNames()).toHaveLength(1); + } finally { + errorSpy.mockRestore(); + } + }); + + test("a malformed container is not an entry, so it is not salvaged", () => { + configWith({ routingProfiles: [] }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + expect(loadConfig()).toEqual(getDefaultConfig()); + } finally { + errorSpy.mockRestore(); + } + }); + }); + test("provider names reject namespace-breaking and reserved object keys", () => { expect(isValidProviderName("openrouter")).toBe(true); expect(isValidProviderName("ollama-cloud")).toBe(true); From fcf9ccd70c56ae930b8f9d0496a79ed2d3277a98 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:46:42 +0900 Subject: [PATCH 2/3] fix(config): make salvage iterative, cover diagnostics, and redact entry ids Four blockers from review, all reproduced against this branch: 1. configDiagnosticsFromRaw still returned getDefaultConfig() with source "fallback". loadConfig salvaged, diagnostics did not, and its own comment says that result can be persisted over the operator's providers and keys. It now runs the same salvage. The validated config comes from the merged candidate while the second argument stays the RAW document minus the same dropped entries, so an absent optional setting is still distinguishable from an injected default. 2. Salvage was one-pass. The sections are not independent of each other, so dropping an invalid combo can expose a profile that referenced it; the second parse failure then discarded the whole config. Salvage now repeats until the document parses, bounded by the number of salvageable entries, and stops as soon as a pass removes nothing. 3. Entry ids are operator-chosen and can be pasted secrets, and both the dropped list and the serialized issue paths went to console.error raw. Dynamic components now go through redactSecretString; static section names stay readable because they are the part that tells the operator where to look. 4. No regression proved the fallback does not weaken the management auth boundary. Added one: on a salvaged config, /api/config still returns 401 anonymously, 401 for the data-plane token, 401 for a wrong admin token, and 200 for the valid admin token, with the salvaged providers intact. Also found while testing: salvage must refuse Codex account namespace collisions. That finding is a relationship between a combo and an account selector but is reported on the combo, so dropping the combo made the document parse and quietly admitted the selector the schema had just refused. Those issues are now unsalvageable and fail the whole document, which is what the two existing namespace-collision tests were asserting. --- src/config.ts | 147 +++++++++++++++++++++++++-- tests/config.test.ts | 73 +++++++++++++ tests/server-management-auth.test.ts | 48 ++++++++- 3 files changed, 256 insertions(+), 12 deletions(-) diff --git a/src/config.ts b/src/config.ts index 02f9a9386d..865beec096 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2176,12 +2176,11 @@ export function loadConfig(): OcxConfig { // in an independent section, drop exactly those and keep the rest. Falling // back to defaults here would silently retire the operator's providers, // keys and prices over a mistake in one routing profile. - const salvaged = dropInvalidConfigSections(merged, retryResult.error); + const salvaged = salvageConfigCandidate(merged, retryResult.error); if (salvaged) { - const salvagedResult = configSchema.safeParse(salvaged.candidate); - if (salvagedResult.success) { - warnDroppedConfigSections(configPath, salvaged.dropped, retryResult.error); - const config = normalizeApiKeyIds(salvagedResult.data as OcxConfig); + { + warnDroppedConfigSections(configPath, salvaged.dropped, salvaged.issues); + const config = normalizeApiKeyIds(salvaged.parsed); warnDegradedHostname(parsed, config); warnDegradedApiKeys(parsed, config); warnDegradedCodexAccountPriorities(parsed, config); @@ -2471,11 +2470,25 @@ function configDiagnosticsFromRaw(raw: string): ConfigDiagnostics { return validFileConfigDiagnostics(normalizeApiKeyIds(result.data as OcxConfig), parsed); } - const retryResult = configSchema.safeParse(mergeConfigDefaults(parsed)); + const merged = mergeConfigDefaults(parsed); + const retryResult = configSchema.safeParse(merged); if (retryResult.success) { return validFileConfigDiagnostics(normalizeApiKeyIds(retryResult.data as OcxConfig), parsed); } + // Same salvage as loadConfig. Without it a single invalid routing profile makes + // diagnostics report the built-in defaults as the config, and a later config write + // persists those defaults over the operator's providers, keys and prices. + // + // The validated config comes from the merged candidate, but the second argument stays + // the RAW document (minus the same dropped entries) so callers can still distinguish an + // absent optional setting from one we injected. + const salvaged = salvageConfigCandidate(merged, retryResult.error, parsed); + if (salvaged) { + warnDroppedConfigSections(getConfigPath(), salvaged.dropped, salvaged.issues); + return validFileConfigDiagnostics(normalizeApiKeyIds(salvaged.parsed), salvaged.rawCandidate); + } + return { config: getDefaultConfig(), source: "fallback", error: schemaDiagnosticsError(result.error) }; } catch { return { config: getDefaultConfig(), source: "fallback", error: "invalid_json" }; @@ -3484,6 +3497,7 @@ function dropInvalidConfigSections( const doomed = new Map>(); for (const issue of error.issues) { + if (isUnsalvageableIssue(issue)) return null; const [section, id] = issue.path; if (typeof section !== "string" || typeof id !== "string") return null; if (!(SALVAGEABLE_CONFIG_SECTIONS as readonly string[]).includes(section)) return null; @@ -3511,14 +3525,127 @@ function dropInvalidConfigSections( return dropped.length > 0 ? { candidate, dropped } : null; } -function warnDroppedConfigSections(configPath: string, dropped: string[], error: z.ZodError): void { +/** + * Salvage until the document parses, not just once. + * + * One pass is not enough because the sections depend on each other: routing + * profiles are validated against the combo map, so dropping an invalid combo can + * expose a profile that referenced it. A single-pass salvage sees that second + * failure and gives up, discarding the whole config -- the exact outcome this + * code exists to prevent. + * + * `rawDocument` is the operator's document before defaults were merged in. When + * supplied, the same entries are deleted from it too, so a diagnostics caller can + * still tell an absent optional setting from one we injected. + */ + +/** + * Findings that must never be salvaged away. + * + * Salvage removes the entry a finding blamed, which is right for an ordinary + * validation mistake and wrong for a namespace collision: the collision is a + * *relationship* between a combo/profile and a Codex account selector, and it is + * reported on the combo. Dropping that combo makes the document parse and quietly + * admits the account selector the schema just refused, turning a hard admission + * boundary into a config that loads. Refuse the whole document instead. + */ +const UNSALVAGEABLE_ISSUE_MESSAGES: readonly string[] = [ + CODEX_ACCOUNT_NAMESPACE_COMBO_ALIAS_COLLISION_ERROR, +]; + +function isUnsalvageableIssue(issue: z.ZodIssue): boolean { + return UNSALVAGEABLE_ISSUE_MESSAGES.some(message => issue.message.includes(message)); +} +function salvageConfigCandidate( + merged: unknown, + initialError: z.ZodError, + rawDocument?: unknown, +): { + candidate: Record; + rawCandidate: unknown; + parsed: OcxConfig; + dropped: string[]; + issues: z.ZodIssue[]; +} | null { + let candidate: unknown = merged; + let rawCandidate: unknown = rawDocument; + let error = initialError; + const dropped: string[] = []; + const issues: z.ZodIssue[] = []; + // Bounded by construction: every pass must remove at least one entry, and there + // are only so many entries to remove. + const budget = countSalvageableEntries(merged) + 1; + for (let pass = 0; pass < budget; pass++) { + const step = dropInvalidConfigSections(candidate, error); + if (!step || step.dropped.length === 0) return null; + dropped.push(...step.dropped); + issues.push(...error.issues); + candidate = step.candidate; + rawCandidate = deleteEntryPaths(rawCandidate, step.dropped); + const result = configSchema.safeParse(candidate); + if (result.success) { + return { candidate: step.candidate, rawCandidate, parsed: result.data as OcxConfig, dropped, issues }; + } + error = result.error; + } + return null; +} + +function countSalvageableEntries(document: unknown): number { + if (!document || typeof document !== "object" || Array.isArray(document)) return 0; + let total = 0; + for (const section of SALVAGEABLE_CONFIG_SECTIONS) { + const value = (document as Record)[section]; + if (value && typeof value === "object" && !Array.isArray(value)) { + total += Object.keys(value as Record).length; + } + } + return total; +} + +/** Delete `section.id` entries from a copy of the raw document. */ +function deleteEntryPaths(document: unknown, entryPaths: readonly string[]): unknown { + if (!document || typeof document !== "object" || Array.isArray(document)) return document; + const next: Record = { ...(document as Record) }; + for (const entryPath of entryPaths) { + const separator = entryPath.indexOf("."); + if (separator <= 0) continue; + const section = entryPath.slice(0, separator); + const id = entryPath.slice(separator + 1); + const container = next[section]; + if (!container || typeof container !== "object" || Array.isArray(container)) continue; + const kept: Record = { ...(container as Record) }; + delete kept[id]; + next[section] = kept; + } + return next; +} + +/** + * Entry ids are operator-chosen and can be token-shaped, so nothing dynamic reaches + * the log unredacted. Static section names stay readable -- they are the part that + * tells the operator where to look. + */ +function redactEntryPath(entryPath: string): string { + const separator = entryPath.indexOf("."); + if (separator <= 0) return redactSecretString(entryPath); + return entryPath.slice(0, separator) + "." + redactSecretString(entryPath.slice(separator + 1)); +} + +function redactIssuePath(path: readonly PropertyKey[]): string { + return path + .map((segment, index) => (index === 0 && typeof segment === "string" ? segment : redactSecretString(String(segment)))) + .join("."); +} + +function warnDroppedConfigSections(configPath: string, dropped: string[], issues: readonly z.ZodIssue[]): void { if (warnedConfigFallbacks.has(configPath)) return; warnedConfigFallbacks.add(configPath); - const reasons = error.issues - .map(issue => `${issue.path.join(".")}: ${issue.message}`) + const reasons = issues + .map(issue => `${redactIssuePath(issue.path)}: ${redactSecretString(issue.message)}`) .join("; "); console.error( - `opencodex config at ${configPath}: dropped [${dropped.join(", ")}] and loaded the rest — ${reasons}. ` + `opencodex config at ${configPath}: dropped [${dropped.map(redactEntryPath).join(", ")}] and loaded the rest — ${reasons}. ` + "Everything else in your config, including providers and modelCosts, is preserved.", ); } diff --git a/tests/config.test.ts b/tests/config.test.ts index 009acb08b0..d9df6c12b4 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1632,6 +1632,79 @@ describe("opencodex config defaults", () => { } }); + + test("diagnostics salvage the same way instead of reporting defaults", () => { + // The salvage in loadConfig was not enough on its own: readConfigDiagnostics fell + // through to getDefaultConfig(), and a config command writing that result back would + // have persisted built-in defaults over the operator's providers, keys and prices. + configWith({ + routingProfiles: { + good: { candidates: [{ provider: "keep1", model: "m" }] }, + bad: { candidates: [{ provider: "TR", model: "moonshotai/kimi-k3" }] }, + }, + }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const diagnostics = readConfigDiagnostics(); + + expect(diagnostics.source).toBe("file"); + expect(diagnostics.error).toBeNull(); + const config = diagnostics.config as Record; + expect(Object.keys(config.providers)).toEqual(expect.arrayContaining(["TR", "keep1", "keep2"])); + expect(config.modelCosts).toHaveProperty("keep1/m"); + expect(Object.keys(config.routingProfiles)).toEqual(["good"]); + } finally { + errorSpy.mockRestore(); + } + }); + + test("salvage repeats when dropping one entry exposes a new failure", () => { + // Sections are not independent of each other: a profile alias is validated against the + // combo map, so removing an invalid combo can surface a NEW failure in a profile that + // was fine while that combo existed. A single-pass salvage saw that second failure and + // discarded the whole config -- the outcome this exists to stop. + configWith({ + combos: { + badCombo: { members: [{ provider: "nope-not-configured", model: "m" }] }, + }, + routingProfiles: { + alsoBad: { candidates: [{ provider: "TR", model: "m" }] }, + good: { candidates: [{ provider: "keep2", model: "m" }] }, + }, + }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const loaded = loadConfig() as Record; + + // Everything unrelated survives, and both bad entries are gone. + expect(Object.keys(loaded.providers)).toEqual(expect.arrayContaining(["TR", "keep1", "keep2"])); + expect(loaded.modelCosts).toHaveProperty("keep1/m"); + expect(Object.keys(loaded.combos ?? {})).not.toContain("badCombo"); + expect(Object.keys(loaded.routingProfiles ?? {})).toEqual(["good"]); + } finally { + errorSpy.mockRestore(); + } + }); + + test("a token-shaped entry id is not echoed into the warning", () => { + // Entry ids are operator-chosen and can be pasted secrets. The warning names the + // section so the operator knows where to look, but nothing dynamic goes out raw. + const tokenId = "sk-ant-api03-" + "A".repeat(40); + configWith({ + routingProfiles: { + [tokenId]: { candidates: [{ provider: "TR", model: "m" }] }, + }, + }); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + loadConfig(); + const logged = errorSpy.mock.calls.map(call => String(call[0])).join("\n"); + expect(logged).toContain("routingProfiles."); + expect(logged).not.toContain(tokenId); + } finally { + errorSpy.mockRestore(); + } + }); test("combos are salvaged the same way", () => { configWith({ combos: { diff --git a/tests/server-management-auth.test.ts b/tests/server-management-auth.test.ts index 0c04384619..335392ea8d 100644 --- a/tests/server-management-auth.test.ts +++ b/tests/server-management-auth.test.ts @@ -1,9 +1,9 @@ -import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; import { SERVER_BUDGET_MS } from "./helpers/test-budget"; import { mkdtempSync, readFileSync, rmSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { saveConfig } from "../src/config"; +import { getConfigPath, saveConfig } from "../src/config"; import { startServer } from "../src/server"; import type { OcxConfig } from "../src/types"; import { serveGuiFile, serveSessionBootstrap } from "../src/server/gui-static"; @@ -586,6 +586,50 @@ describe("management and data-plane credential separation", () => { } }); + + test("config salvage does not weaken the management auth boundary (#1785)", async () => { + // Salvage keeps a config loading after dropping an invalid entry. That must not turn into + // a way to reach the management plane: the credential separation is enforced before route + // dispatch, and a partially-salvaged config has to behave exactly like a clean one. + const salvageable = remoteConfig() as Record; + salvageable.routingProfiles = { + good: { candidates: [{ provider: "test", model: "gpt-test" }] }, + bad: { candidates: [{ provider: "not-configured", model: "gpt-test" }] }, + }; + writeFileSync(getConfigPath(), JSON.stringify(salvageable, null, 2), { mode: 0o600 }); + + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + const server = startServer(0); + try { + const anonymous = await fetch(new URL("/api/config", server.url)); + expect(anonymous.status).toBe(401); + + const withDataToken = await fetch(new URL("/api/config", server.url), { + headers: { "x-opencodex-api-key": "data-secret" }, + }); + expect(withDataToken.status).toBe(401); + + const withWrongAdminToken = await fetch(new URL("/api/config", server.url), { + headers: { "x-opencodex-api-key": "not-the-admin-secret" }, + }); + expect(withWrongAdminToken.status).toBe(401); + + const withAdminToken = await fetch(new URL("/api/config", server.url), { + headers: { "x-opencodex-api-key": "admin-secret" }, + }); + expect(withAdminToken.status).toBe(200); + + // The salvaged config is what the authorized caller sees: the provider survives and only + // the invalid profile is gone. A fallback here would hand back built-in defaults, which a + // later write would persist over the operator's providers. + const body = await withAdminToken.json() as Record; + expect(Object.keys(body.providers ?? {})).toContain("test"); + expect(Object.keys(body.routingProfiles ?? {})).not.toContain("bad"); + } finally { + await server.stop(true); + errorSpy.mockRestore(); + } + }); test("a management token that matches the data environment token closes only the management plane", async () => { process.env.OPENCODEX_ADMIN_AUTH_TOKEN = "data-secret"; saveConfig(remoteConfig()); From 3c550a24dda76f77ae611cd01a247d4308346039 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:53:03 +0900 Subject: [PATCH 3/3] fix(config): keep the operator's config in diagnostics without hiding the error Follow-up to the previous commit. The first attempt made configDiagnosticsFromRaw return source "file" with error null after a successful salvage, which broke two persisted-combo tests -- correctly. Diagnostics is the surface that TELLS callers the file is invalid, and provider reload, catalog sync, cost reconcile and codex admission all gate on exactly source !== "file" || error !== null. Salvaging the error away would have quietly admitted configs those paths must refuse. The real #1785 defect is narrower: the config PAYLOAD was getDefaultConfig(), so a caller that writes diagnostics back persists factory defaults over the operator's providers, keys and prices. Now source stays "fallback" and error keeps the real schema message, and only the payload changes -- it carries the salvaged document instead of defaults. Full suite on the previous head caught this; the two combo tests are restored to passing without weakening what they assert. --- src/config.ts | 24 +++++++++++++++--------- tests/config.test.ts | 21 ++++++++++++++------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/config.ts b/src/config.ts index 865beec096..3935e3dc51 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2476,17 +2476,23 @@ function configDiagnosticsFromRaw(raw: string): ConfigDiagnostics { return validFileConfigDiagnostics(normalizeApiKeyIds(retryResult.data as OcxConfig), parsed); } - // Same salvage as loadConfig. Without it a single invalid routing profile makes - // diagnostics report the built-in defaults as the config, and a later config write - // persists those defaults over the operator's providers, keys and prices. + // #1785: one invalid routing profile must not make diagnostics report the built-in + // defaults AS the config, because a later config write persists those defaults over the + // operator's providers, keys and prices. // - // The validated config comes from the merged candidate, but the second argument stays - // the RAW document (minus the same dropped entries) so callers can still distinguish an - // absent optional setting from one we injected. - const salvaged = salvageConfigCandidate(merged, retryResult.error, parsed); + // The failure is still reported. `source` stays "fallback" and `error` keeps the real + // schema message -- diagnostics is the surface that tells callers the file is invalid, + // and every consumer that must refuse an invalid config (provider reload, catalog sync, + // cost reconcile, codex admission) gates on exactly those two fields. Only `config` + // changes: it carries the salvaged document instead of factory defaults, so a caller + // that ignores the error and writes it back preserves what the operator configured. + const salvaged = salvageConfigCandidate(merged, retryResult.error); if (salvaged) { - warnDroppedConfigSections(getConfigPath(), salvaged.dropped, salvaged.issues); - return validFileConfigDiagnostics(normalizeApiKeyIds(salvaged.parsed), salvaged.rawCandidate); + return { + config: normalizeApiKeyIds(salvaged.parsed), + source: "fallback", + error: schemaDiagnosticsError(result.error), + }; } return { config: getDefaultConfig(), source: "fallback", error: schemaDiagnosticsError(result.error) }; diff --git a/tests/config.test.ts b/tests/config.test.ts index d9df6c12b4..1cd7d810cc 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1633,10 +1633,14 @@ describe("opencodex config defaults", () => { }); - test("diagnostics salvage the same way instead of reporting defaults", () => { - // The salvage in loadConfig was not enough on its own: readConfigDiagnostics fell - // through to getDefaultConfig(), and a config command writing that result back would - // have persisted built-in defaults over the operator's providers, keys and prices. + test("diagnostics keep the operator's config instead of reporting defaults", () => { + // The salvage in loadConfig was not enough on its own. readConfigDiagnostics returned + // getDefaultConfig(), and a config command writing that result back would have persisted + // built-in defaults over the operator's providers, keys and prices. + // + // The failure is still REPORTED -- source stays "fallback" and error keeps the schema + // message, which is what provider reload, catalog sync, cost reconcile and codex admission + // gate on. Only the config payload changes. configWith({ routingProfiles: { good: { candidates: [{ provider: "keep1", model: "m" }] }, @@ -1647,12 +1651,15 @@ describe("opencodex config defaults", () => { try { const diagnostics = readConfigDiagnostics(); - expect(diagnostics.source).toBe("file"); - expect(diagnostics.error).toBeNull(); + // Still invalid, and still says why. + expect(diagnostics.source).toBe("fallback"); + expect(diagnostics.error).toContain("is disabled"); + + // But the payload is the operator's config, not the factory defaults. const config = diagnostics.config as Record; expect(Object.keys(config.providers)).toEqual(expect.arrayContaining(["TR", "keep1", "keep2"])); expect(config.modelCosts).toHaveProperty("keep1/m"); - expect(Object.keys(config.routingProfiles)).toEqual(["good"]); + expect(Object.keys(config.routingProfiles ?? {})).toEqual(["good"]); } finally { errorSpy.mockRestore(); }