From 071787ad9395080c03cff52922d3389f6f48d9b9 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Fri, 28 Aug 2026 23:18:53 +0900 Subject: [PATCH 1/8] fix(grok): preserve retired orphan model tables (cherry picked from commit 0df51fd5af2d67720cbd2d0a6576fb01b1a731ab) --- src/grok/inject.ts | 6 +++++- tests/grok-orphan-adoption.test.ts | 34 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/grok/inject.ts b/src/grok/inject.ts index fee7e60180..4636470f47 100644 --- a/src/grok/inject.ts +++ b/src/grok/inject.ts @@ -395,7 +395,11 @@ export function injectGrokConfig( // Adopt our own pre-fence entries (#511) BEFORE reserving user aliases, so the stale // duplicate is replaced instead of routed around forever. Runs inside the normalized // window so the user's dominant EOL is still restored below. - const orphans = findOpencodexOrphans(originalContent, originalRegion); + // Use the full catalog, not the emitted subset: an explicitly excluded current model must + // still lose its stale unfenced table, or that table would bypass the user's exclusion. + const catalogModelIds = new Set(models.map(model => model.id)); + const orphans = findOpencodexOrphans(originalContent, originalRegion) + .filter(orphan => orphan.modelId !== undefined && catalogModelIds.has(orphan.modelId)); const content = removeOrphanTables(originalContent, orphans); // Removing bytes above the fence MOVES it: recompute rather than adjust arithmetic, // so the splice below cannot cut the file in the wrong place. diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index aba8af6e27..e05719ba2e 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -150,6 +150,9 @@ describe("Grok orphan adoption (#511)", () => { "[models]", 'default = "ocx-retired"', "", + "[ui]", + 'fork_secondary_model = "ocx-retired"', + "", "[model.ocx-retired]", 'model = "retired/model"', 'base_url = "http://127.0.0.1:10100/v1"', @@ -160,6 +163,37 @@ describe("Grok orphan adoption (#511)", () => { injectGrokConfig(10100, MODELS, { grokHome }); const content = readFileSync(configPath, "utf8"); expect(content).toContain('default = "ocx-retired"'); + expect(content).toContain('fork_secondary_model = "ocx-retired"'); + expect(content).toContain("[model.ocx-retired]"); + expect(content).toContain('model = "retired/model"'); + + const second = injectGrokConfig(10100, MODELS, { grokHome }); + expect(second).toMatchObject({ ok: true, changed: false }); + expect(readFileSync(configPath, "utf8")).toBe(content); + }); + + test("keeps an owned-looking orphan whose model id is missing", () => { + writeFileSync(configPath, [ + "[model.ocx-unknown]", + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join("\n")); + + injectGrokConfig(10100, MODELS, { grokHome }); + const content = readFileSync(configPath, "utf8"); + expect(content).toContain("[model.ocx-unknown]"); + expect(content).toContain('api_key = "opencodex-loopback"'); + }); + + test("still removes a catalog orphan when that model is excluded", () => { + writeOrphanedConfig(); + + injectGrokConfig(10100, MODELS, { + grokHome, + excluded: new Set(["gpt-5.6-sol"]), + }); + expect(modelTables(readFileSync(configPath, "utf8"))).toEqual([]); }); // F7: the sweep must converge, or `changed` is meaningless to callers. From c8534f631ee2c3d3275a545f4924f68e4cf61874 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 00:12:24 +0900 Subject: [PATCH 2/8] fix(grok): close orphan lifecycle review gaps (cherry picked from commit 9a9bace3278e6ac801564860dc23a3fd0ce04e05) --- src/grok/inject.ts | 79 +++++++++---- src/grok/sync.ts | 11 +- .../management/native-integration-routes.ts | 13 ++- tests/grok-orphan-adoption.test.ts | 110 +++++++++++++++++- tests/grok-sync.test.ts | 62 +++++++++- tests/native-grok-toggle.test.ts | 23 +++- 6 files changed, 261 insertions(+), 37 deletions(-) diff --git a/src/grok/inject.ts b/src/grok/inject.ts index 4636470f47..e19192c583 100644 --- a/src/grok/inject.ts +++ b/src/grok/inject.ts @@ -144,9 +144,11 @@ function tableBodyKeys(body: string): Map { const match = /^[ \t]*([A-Za-z0-9_-]+)[ \t]*=[ \t]*(.*?)[ \t]*$/.exec(line); if (!match) continue; const raw = match[2]!; - const value = raw.startsWith('"') && raw.endsWith('"') && raw.length >= 2 + const value = raw.length >= 2 && raw.startsWith('"') && raw.endsWith('"') ? decodeTomlBasicString(raw.slice(1, -1)) - : raw; + : raw.length >= 2 && raw.startsWith("'") && raw.endsWith("'") + ? raw.slice(1, -1) // TOML literal strings do not process escapes. + : raw; if (!keys.has(match[1]!)) keys.set(match[1]!, value); } return keys; @@ -344,7 +346,13 @@ export function buildGrokManagedBlock( export function injectGrokConfig( port: number, models: GrokInjectModel[], - opts: { grokHome?: string; hostname?: string; excluded?: ReadonlySet } = {}, + opts: { + grokHome?: string; + hostname?: string; + excluded?: ReadonlySet; + /** Unfiltered known ids used only to distinguish hidden current models from retired ones. */ + catalogModelIds?: ReadonlySet; + } = {}, ): GrokInjectResult { const grokHome = resolveGrokHome(opts.grokHome); if (!isDirectory(grokHome)) { @@ -395,9 +403,11 @@ export function injectGrokConfig( // Adopt our own pre-fence entries (#511) BEFORE reserving user aliases, so the stale // duplicate is replaced instead of routed around forever. Runs inside the normalized // window so the user's dominant EOL is still restored below. - // Use the full catalog, not the emitted subset: an explicitly excluded current model must - // still lose its stale unfenced table, or that table would bypass the user's exclusion. - const catalogModelIds = new Set(models.map(model => model.id)); + // Use the full UNFILTERED catalog, not the emitted subset: explicitly excluded and otherwise + // hidden current models must still lose stale unfenced tables, or those tables would bypass + // the user's visibility choice. Direct callers that do not have a separate catalog keep the + // historical `models` behavior. + const catalogModelIds = opts.catalogModelIds ?? new Set(models.map(model => model.id)); const orphans = findOpencodexOrphans(originalContent, originalRegion) .filter(orphan => orphan.modelId !== undefined && catalogModelIds.has(orphan.modelId)); const content = removeOrphanTables(originalContent, orphans); @@ -481,29 +491,52 @@ export function stripGrokConfig(opts: { grokHome?: string } = {}): GrokInjectRes const rawContent = readFileSync(configPath, "utf8"); const eol = dominantEol(rawContent); const content = applyEol(rawContent, "\n"); - const region = findManagedRegion(content); - if (!region) { - return { ok: true, changed: false, message: "No opencodex managed block found in Grok config." }; + const originalRegion = findManagedRegion(content); + if (originalRegion?.orphaned) return orphanedMarkerResult("cleanup"); + + // Remove the fence against its ORIGINAL offsets first. A pre-fence orphan's span is clamped + // at the fence start and can include the separator newline injection added. Sweeping that + // orphan first and then applying this separator undo would remove one additional USER newline. + let stripped: string; + let orphanCount = 0; + if (originalRegion) { + let removalEnd = originalRegion.end; + if (content.startsWith("\n", removalEnd)) removalEnd += 1; + let prefix = content.slice(0, originalRegion.start); + const restOfFile = content.slice(removalEnd); + // Undo the single separator newline injection added. Two cases, mirroring inject: + // "X\n" -> "X\n" + "\n" + block => prefix ends "\n\n", drop one. + // "X" -> "X" + "\n" + block => prefix ends "\n" at EOF, drop it. + // A block the user has appended content after is left alone: we never shrink their bytes. + if (prefix.endsWith("\n\n")) prefix = prefix.slice(0, -1); + else if (restOfFile.length === 0 && prefix.endsWith("\n")) prefix = prefix.slice(0, -1); + // Keep the old fence boundary while sweeping. Concatenating first would let the last + // pre-fence orphan absorb comment-only or bare-key user content appended after the fence. + const prefixOrphans = findOpencodexOrphans(prefix, null); + const tailOrphans = findOpencodexOrphans(restOfFile, null); + orphanCount = prefixOrphans.length + tailOrphans.length; + stripped = removeOrphanTables(prefix, prefixOrphans) + + removeOrphanTables(restOfFile, tailOrphans); + } else { + // Retired or otherwise non-emitted OpenCodex tables may intentionally remain outside the + // fence while the integration is enabled. Teardown owns those strictly identified tables + // even after Grok has re-serialized the file and dropped our marker comments. + const orphans = findOpencodexOrphans(content, null); + if (orphans.length === 0) { + return { ok: true, changed: false, message: "No opencodex managed block found in Grok config." }; + } + orphanCount = orphans.length; + stripped = removeOrphanTables(content, orphans); } - if (region.orphaned) return orphanedMarkerResult("cleanup"); - - let removalEnd = region.end; - if (content.startsWith("\n", removalEnd)) removalEnd += 1; - let prefix = content.slice(0, region.start); - const restOfFile = content.slice(removalEnd); - // Undo the single separator newline injection added. Two cases, mirroring inject: - // "X\n" -> "X\n" + "\n" + block => prefix ends "\n\n", drop one. - // "X" -> "X" + "\n" + block => prefix ends "\n" at EOF, drop it. - // A block the user has appended content after is left alone: we never shrink their bytes. - if (prefix.endsWith("\n\n")) prefix = prefix.slice(0, -1); - else if (restOfFile.length === 0 && prefix.endsWith("\n")) prefix = prefix.slice(0, -1); - const stripped = prefix + restOfFile; + if (orphanCount > 0) copyBackupOnce(configPath, join(grokHome, "config.toml.bak-opencodex")); atomicWriteFile(configPath, applyEol(stripped, eol)); return { ok: true, changed: true, - message: "Removed the opencodex managed block from Grok config.", + message: originalRegion + ? "Removed the opencodex managed block from Grok config." + : "Removed stale opencodex-managed model entries from Grok config.", }; } catch (error) { return errorResult("strip", error); diff --git a/src/grok/sync.ts b/src/grok/sync.ts index 6e24b528fb..3d2957f4f4 100644 --- a/src/grok/sync.ts +++ b/src/grok/sync.ts @@ -6,7 +6,7 @@ * * Deps are injectable (mirrors src/codex/sync.ts) so tests can run without a live proxy. */ -import { visibleNativeSlugs, filterCatalogVisibleModels, nativeContextLimits, nativeOpenAiContextWindow, type CatalogModel } from "../codex/catalog"; +import { visibleNativeSlugs, filterCatalogVisibleModels, nativeContextLimits, nativeOpenAiContextWindow, nativeOpenAiSlugs, type CatalogModel } from "../codex/catalog"; import type { OcxConfig } from "../types"; import { injectGrokConfig, type GrokInjectModel, type GrokInjectResult } from "./inject"; @@ -33,8 +33,14 @@ export async function syncGrokConfig( deps: GrokSyncDeps = { fetchAllModels: defaultFetchAllModels, injectGrokConfig }, ): Promise { let models: GrokInjectModel[]; + let catalogModelIds: Set; try { - const routed = filterCatalogVisibleModels(await deps.fetchAllModels(config), config); + const allRouted = await deps.fetchAllModels(config); + const routed = filterCatalogVisibleModels(allRouted, config); + catalogModelIds = new Set([ + ...nativeOpenAiSlugs(), + ...allRouted.map(model => model.alias ?? `${model.provider}/${model.id}`), + ]); models = [ // Native slugs carry their context window too. Without it Grok falls back to its own // default (200k) and understates models like gpt-5.6-sol, which is 372k. This is the same @@ -62,5 +68,6 @@ export async function syncGrokConfig( ...(opts.hostname !== undefined ? { hostname: opts.hostname } : {}), ...(opts.grokHome !== undefined ? { grokHome: opts.grokHome } : {}), excluded: new Set(config.grokExcludedModels ?? []), + catalogModelIds, }); } diff --git a/src/server/management/native-integration-routes.ts b/src/server/management/native-integration-routes.ts index 06c4bbfb3b..357971a525 100644 --- a/src/server/management/native-integration-routes.ts +++ b/src/server/management/native-integration-routes.ts @@ -19,7 +19,7 @@ */ import { loadConfig, saveConfigPreservingClaudeCode } from "../../config"; import { readRuntimePort } from "../../config/process-state"; -import { desktopVisibleNativeSlugs, filterCatalogVisibleModels, nativeContextLimits, nativeOpenAiContextWindow, visibleNativeSlugs } from "../../codex/catalog"; +import { desktopVisibleNativeSlugs, filterCatalogVisibleModels, nativeContextLimits, nativeOpenAiContextWindow, nativeOpenAiSlugs, visibleNativeSlugs } from "../../codex/catalog"; import { providerContextCap } from "../../providers/context-cap"; import { OPENAI_CODEX_PROVIDER_ID } from "../../providers/openai-tiers"; import { inspectDesktop3pConfigLibrary, removeDesktop3pStandardPivot, writeDesktop3pConfig } from "../../claude/desktop-3p"; @@ -503,8 +503,14 @@ async function handleGrokToggle(ctx: ManagementContext): Promise { */ const fetchModels = deps.fetchAllModels ?? defaultFetchAllModels; let models: GrokInjectModel[]; + let catalogModelIds: Set; try { - const routed = filterCatalogVisibleModels(await fetchModels(config), config); + const allRouted = await fetchModels(config); + const routed = filterCatalogVisibleModels(allRouted, config); + catalogModelIds = new Set([ + ...nativeOpenAiSlugs(), + ...allRouted.map(model => model.alias ?? `${model.provider}/${model.id}`), + ]); models = [ // Native slugs carry their context window: without it Grok falls back // to its own 200k default and understates a 372k model. @@ -535,6 +541,9 @@ async function handleGrokToggle(ctx: ManagementContext): Promise { // writer allocates aliases over everything, so a model's alias never // depends on its neighbours' switches. excluded: new Set(config.grokExcludedModels ?? []), + // Visibility filters decide what to emit, not whether an owned pre-fence table is still + // current. Otherwise a hidden model is mistaken for retired state and survives outside. + catalogModelIds, }); if (result.skippedReason === "non-loopback") { diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index e05719ba2e..b7d7876a5b 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { injectGrokConfig } from "../src/grok/inject"; +import { injectGrokConfig, stripGrokConfig } from "../src/grok/inject"; /** * #511 — Grok Build reported 200k for every model. @@ -180,12 +180,118 @@ describe("Grok orphan adoption (#511)", () => { "", ].join("\n")); - injectGrokConfig(10100, MODELS, { grokHome }); + const result = injectGrokConfig(10100, MODELS, { grokHome }); + expect(result).toMatchObject({ ok: true, changed: true }); const content = readFileSync(configPath, "utf8"); expect(content).toContain("[model.ocx-unknown]"); expect(content).toContain('api_key = "opencodex-loopback"'); }); + test("removes a hidden current orphan but preserves a genuinely retired one", () => { + writeFileSync(configPath, [ + "[model.ocx-hidden]", + 'model = "hidden/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join("\n")); + + const result = injectGrokConfig(10100, MODELS, { + grokHome, + catalogModelIds: new Set(["gpt-5.6-sol", "hidden/model"]), + }); + expect(result).toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + expect(content).not.toContain("[model.ocx-hidden]"); + expect(content).not.toContain('model = "hidden/model"'); + expect(content).toContain("[model.ocx-retired]"); + expect(content).toContain('model = "retired/model"'); + }); + + test("adopts a current orphan whose TOML model id uses literal quotes", () => { + writeOrphanedConfig(); + writeFileSync( + configPath, + readFileSync(configPath, "utf8").replace('model = "gpt-5.6-sol"', "model = 'gpt-5.6-sol'"), + ); + + const result = injectGrokConfig(10100, MODELS, { grokHome }); + expect(result).toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + expect(modelTables(content)).toEqual(["ocx-gpt-5-6-sol"]); + expect(content).not.toContain("model = 'gpt-5.6-sol'"); + expect(content).toContain('model = "gpt-5.6-sol"'); + }); + + test("teardown removes a preserved retired orphan and restores user bytes exactly", () => { + for (const eol of ["\n", "\r\n"]) { + const userPrefix = [`theme = "${eol === "\n" ? "lf" : "crlf"}"`, "", ""].join(eol); + const orphan = [ + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join(eol); + writeFileSync(configPath, userPrefix + orphan); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toContain("[model.ocx-retired]"); + + const stripped = stripGrokConfig({ grokHome }); + expect(stripped).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(userPrefix); + } + }); + + test("teardown preserves a comment-only tail beyond the fence byte-for-byte", () => { + for (const eol of ["\n", "\r\n"]) { + const userPrefix = [`theme = "${eol === "\n" ? "lf" : "crlf"}"`, "", ""].join(eol); + const orphan = [ + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join(eol); + const tail = ["# keep this post-fence note", "bare_user_key = true", ""].join(eol); + writeFileSync(configPath, userPrefix + orphan); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + writeFileSync(configPath, readFileSync(configPath, "utf8") + tail); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(userPrefix + tail); + } + }); + + test("markerless teardown removes only ownership-proven orphan tables", () => { + const owned = [ + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join("\n"); + const userOwned = [ + "[model.user-owned]", + 'model = "user/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "not-ours"', + "", + ].join("\n"); + writeFileSync(configPath, owned + userOwned); + + const stripped = stripGrokConfig({ grokHome }); + expect(stripped).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(userOwned); + }); + test("still removes a catalog orphan when that model is excluded", () => { writeOrphanedConfig(); diff --git a/tests/grok-sync.test.ts b/tests/grok-sync.test.ts index 0ed57bcfb9..177fc15f0c 100644 --- a/tests/grok-sync.test.ts +++ b/tests/grok-sync.test.ts @@ -1,10 +1,10 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { injectGrokConfig } from "../src/grok/inject"; +import { injectGrokConfig, type GrokInjectModel } from "../src/grok/inject"; import { syncGrokConfig } from "../src/grok/sync"; -import { nativeOpenAiContextWindow, visibleNativeSlugs } from "../src/codex/catalog"; +import { nativeOpenAiContextWindow, nativeOpenAiSlugs, visibleNativeSlugs } from "../src/codex/catalog"; import type { CatalogModel } from "../src/codex/catalog"; import { resetCodexModelEntitlementCacheForTests, @@ -46,6 +46,62 @@ describe("syncGrokConfig", () => { } }); + test("classifies provider-hidden models from the unfiltered catalog without emitting them", async () => { + const { root, grokHome } = tempGrokHome(); + try { + const config = { + ...baseConfig, + providers: { stub: { selectedModels: ["visible"] } }, + } as unknown as OcxConfig; + const catalog = [ + { id: "visible", provider: "stub" } as CatalogModel, + { id: "hidden", provider: "stub" } as CatalogModel, + ]; + writeFileSync(join(grokHome, "config.toml"), [ + "[model.ocx-stub-hidden]", + 'model = "stub/hidden"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join("\n")); + + const result = await syncGrokConfig(10190, config, { grokHome }, { + fetchAllModels: async () => catalog, + injectGrokConfig, + }); + expect(result).toMatchObject({ ok: true, changed: true }); + const content = readFileSync(join(grokHome, "config.toml"), "utf8"); + expect(content).toContain('model = "stub/visible"'); + expect(content).not.toContain("[model.ocx-stub-hidden]"); + expect(content).not.toContain('model = "stub/hidden"'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + test("keeps disabled native ids in the orphan-classification catalog", async () => { + const hiddenNative = nativeOpenAiSlugs()[0]!; + let emitted: GrokInjectModel[] | undefined; + let catalogModelIds: ReadonlySet | undefined; + const result = await syncGrokConfig( + 10190, + { ...baseConfig, disabledModels: [hiddenNative] } as OcxConfig, + {}, + { + fetchAllModels: async () => [], + injectGrokConfig: ((port: number, models: GrokInjectModel[], opts: Parameters[2]) => { + void port; + emitted = models; + catalogModelIds = opts.catalogModelIds; + return { ok: true, changed: false, message: "captured" }; + }) as typeof injectGrokConfig, + }, + ); + expect(result.ok).toBe(true); + expect(emitted?.some(model => model.id === hiddenNative)).toBe(false); + expect(catalogModelIds?.has(hiddenNative)).toBe(true); + }); + // Native slugs used to be injected as a bare { id }, so no `context_window` line was written // and Grok fell back to its own 200k default — understating gpt-5.6-sol, which is 372k. The // window comes from the same accessor the dashboard uses, so the two surfaces agree. diff --git a/tests/native-grok-toggle.test.ts b/tests/native-grok-toggle.test.ts index 2a405e9e35..5fa24382f9 100644 --- a/tests/native-grok-toggle.test.ts +++ b/tests/native-grok-toggle.test.ts @@ -346,18 +346,24 @@ test("the route never calls syncGrokConfig, and the inspector never re-implement test("the route's model list is byte-identical to syncGrokConfig's", async () => { writeConfig("# user only\n"); - const config = baseConfig({ grokExcludedModels: ["stub/m2"] }); + const config = baseConfig({ + disabledModels: ["stub/m2"], + grokExcludedModels: ["stub/m3"], + }); const catalog = [ { provider: "stub", id: "m1", alias: "fast", contextWindow: 64000 }, { provider: "stub", id: "m2" }, + { provider: "stub", id: "m3" }, ]; let routeModels: GrokInjectModel[] | null = null; let routeExcluded: ReadonlySet | null = null; + let routeCatalogModelIds: ReadonlySet | null = null; const routeDeps = testDeps({ fetchAllModels: (async () => catalog) as never, injectGrokConfig: ((port: number, models: GrokInjectModel[], opts: Parameters[2]) => { routeModels = models; routeExcluded = opts?.excluded ?? null; + routeCatalogModelIds = opts?.catalogModelIds ?? null; return injectGrokConfig(port, models, opts); }) as typeof injectGrokConfig, }); @@ -366,29 +372,36 @@ test("the route's model list is byte-identical to syncGrokConfig's", async () => let syncModels: GrokInjectModel[] | null = null; let syncExcluded: ReadonlySet | null = null; + let syncCatalogModelIds: ReadonlySet | null = null; await syncGrokConfig(10100, config, { hostname: "127.0.0.1" }, { fetchAllModels: (async () => catalog) as never, injectGrokConfig: ((port: number, models: GrokInjectModel[], opts: Parameters[2]) => { syncModels = models; syncExcluded = opts?.excluded ?? null; + syncCatalogModelIds = opts?.catalogModelIds ?? null; return injectGrokConfig(port, models, opts); }) as typeof injectGrokConfig, }); expect(JSON.stringify(routeModels)).toBe(JSON.stringify(syncModels)); + expect(routeCatalogModelIds && [...routeCatalogModelIds].sort()) + .toEqual(syncCatalogModelIds && [...syncCatalogModelIds].sort()); + expect(routeCatalogModelIds?.has("stub/m2")).toBe(true); /* * The exclusion half of the clause (C-gate blocker): the FULL list goes to * the writer together with the exclusion SET, never a pre-filtered list — * dropping `excluded` here would leave the models arrays identical while * excluded models silently leaked into the fence. */ - expect(routeExcluded && [...routeExcluded].sort()).toEqual(["stub/m2"]); - expect(syncExcluded && [...syncExcluded].sort()).toEqual(["stub/m2"]); - // And the exclusion actually reached the fence both times: each write went - // through the real writer into the fixture file, and m2 appears in neither. + expect(routeExcluded && [...routeExcluded].sort()).toEqual(["stub/m3"]); + expect(syncExcluded && [...syncExcluded].sort()).toEqual(["stub/m3"]); + // Visibility and Grok-specific exclusion both reached the fence, while the hidden model + // stayed in the separate classification catalog for stale-orphan cleanup. const fence = readConfig(); expect(fence).toContain('model = "fast"'); expect(fence).not.toContain("stub/m2"); expect(fence).not.toContain("ocx-stub-m2"); + expect(fence).not.toContain("stub/m3"); + expect(fence).not.toContain("ocx-stub-m3"); }); test("a late orphan surfaced by the WRITER still maps to 409, never to absent", () => { From 78b536626f24f31563bf605b8e5eddca0981c0f0 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 00:57:38 +0900 Subject: [PATCH 3/8] fix(grok): harden orphan ownership classification (cherry picked from commit ccba4967e9a292bd2af284e5c5700a0824408ec5) --- src/grok/catalog.ts | 55 +++++++ src/grok/inject.ts | 111 +++++++++++--- src/grok/sync.ts | 33 ++--- .../management/native-integration-routes.ts | 32 ++-- tests/grok-orphan-adoption.test.ts | 137 +++++++++++++++++- tests/grok-sync.test.ts | 79 ++++++++++ tests/native-grok-toggle.test.ts | 28 ++++ 7 files changed, 406 insertions(+), 69 deletions(-) create mode 100644 src/grok/catalog.ts diff --git a/src/grok/catalog.ts b/src/grok/catalog.ts new file mode 100644 index 0000000000..2593a18374 --- /dev/null +++ b/src/grok/catalog.ts @@ -0,0 +1,55 @@ +import { comboPublicModelId } from "../combos"; +import { + filterCatalogVisibleModels, + nativeContextLimits, + nativeOpenAiContextWindow, + nativeOpenAiSlugs, + visibleNativeSlugs, + type CatalogModel, +} from "../codex/catalog"; +import type { OcxConfig } from "../types"; +import type { GrokInjectModel } from "./inject"; + +export interface GrokCatalogProjection { + models: GrokInjectModel[]; + catalogModelIds: ReadonlySet; + disabledProviderNamespaces: ReadonlySet; + comboPublicModelIds: ReadonlySet; +} + +/** + * Project one fetched catalog into both emitted Grok rows and orphan-classification evidence. + * Keeping this shared prevents `ocx start` and the management toggle from disagreeing. + */ +export function projectGrokCatalog( + allRouted: CatalogModel[], + config: OcxConfig, +): GrokCatalogProjection { + const routed = filterCatalogVisibleModels(allRouted, config); + const limits = nativeContextLimits(config); + return { + catalogModelIds: new Set([ + ...nativeOpenAiSlugs(), + ...allRouted.map(model => model.alias ?? `${model.provider}/${model.id}`), + ]), + disabledProviderNamespaces: new Set( + Object.entries(config.providers) + .filter(([, provider]) => provider?.disabled === true) + .map(([name]) => name), + ), + comboPublicModelIds: new Set( + Object.entries(config.combos ?? {}) + .map(([id, combo]) => comboPublicModelId(id, combo)), + ), + models: [ + ...visibleNativeSlugs(config).map(id => { + const contextWindow = nativeOpenAiContextWindow(id, limits); + return { id, ...(contextWindow !== undefined ? { contextWindow } : {}) }; + }), + ...routed.map(model => ({ + id: model.alias ?? `${model.provider}/${model.id}`, + ...(model.contextWindow !== undefined ? { contextWindow: model.contextWindow } : {}), + })), + ], + }; +} diff --git a/src/grok/inject.ts b/src/grok/inject.ts index e19192c583..dc407d8109 100644 --- a/src/grok/inject.ts +++ b/src/grok/inject.ts @@ -121,17 +121,17 @@ function userModelAliases(content: string, region: ManagedRegion | null): Set]` table outside the fence that opencodex itself wrote. */ interface OrphanTable { alias: string; /** The model id this entry routes to — used to find its replacement alias. */ - modelId: string | undefined; + modelId: string; + /** Explicit markers authorize teardown; legacy fingerprints authorize replacement only. */ + ownership: "explicit" | "legacy"; /** Offsets into the NORMALIZED content: header start .. next header start (or EOF). */ start: number; end: number; @@ -164,6 +164,44 @@ function isLoopbackBaseUrl(value: string | undefined): boolean { } } +/** Exact marker emitted inside every modern generated model table. */ +function hasInlineOwnershipMarker(value: string | undefined): boolean { + return value !== undefined + && /^\{[ \t]*["']x-opencodex-grok["'][ \t]*=[ \t]*["']1["'][ \t]*\}$/.test(value); +} + +/** Historical deterministic alias, including collision suffixes allocated by the writer. */ +function isGeneratedAliasForModel(alias: string, modelId: string): boolean { + const base = `ocx-${modelId.replace(/[^A-Za-z0-9_-]/g, "-")}`; + if (alias === base) return true; + if (!alias.startsWith(`${base}-`)) return false; + const suffix = alias.slice(base.length + 1); + return /^[1-9][0-9]*$/.test(suffix) && Number(suffix) >= 2; +} + +/** Pre-marker auto-generated row shape. Manual rows never carried the generated name. */ +function isLegacyGeneratedTable(alias: string, keys: ReadonlyMap): boolean { + const modelId = keys.get("model"); + return modelId !== undefined + && modelId.length > 0 + && keys.get("api_backend") === "chat_completions" + && keys.get("name") === `OCX ${modelId}` + && isGeneratedAliasForModel(alias, modelId); +} + +/** Classify a direct provider/model id without stealing a slash-shaped configured combo alias. */ +function isDisabledProviderModelId( + modelId: string, + disabledProviderNamespaces: ReadonlySet | undefined, + comboPublicModelIds: ReadonlySet | undefined, +): boolean { + if (!disabledProviderNamespaces || comboPublicModelIds?.has(modelId)) return false; + const slash = modelId.indexOf("/"); + return slash > 0 + && slash < modelId.length - 1 + && disabledProviderNamespaces.has(modelId.slice(0, slash)); +} + /** * Model tables OUTSIDE the fence that opencodex itself wrote (#511). * @@ -174,12 +212,16 @@ function isLoopbackBaseUrl(value: string | undefined): boolean { * resolves the original, finds no `context_window`, and falls back to its own 200k. * * Ownership is CONJUNCTIVE and deliberately strict, because a false positive deletes a - * hand-written user model: + * hand-written user model. The public manual recipe intentionally uses the same loopback key, + * endpoint, and Responses backend, so those fields are not ownership proof. We additionally + * require either the durable generated marker or the exact pre-marker legacy fingerprint: * - a plain `[model.x]` header (never `[[model.x]]` / `[model.x.sub]` — those spellings * mark human authorship and stay reserved); * - `api_key` equal to our own literal; * - a loopback `base_url`, so an entry that merely copied our key while pointing at a * remote host is left alone. + * - `x-opencodex-grok = "1"` in generated inline/child extra_headers, OR the historical + * chat_completions + `name = "OCX "` + deterministic generated alias shape. * A loopback base_url ALONE is not enough: aiming your own model at the local proxy is a * legitimate thing to do. */ @@ -213,6 +255,9 @@ function findOpencodexOrphans(content: string, region: ManagedRegion | null): Or const keys = tableBodyKeys(content.slice(header.index + header.length, bodyEnd)); if (keys.get("api_key") !== OPENCODEX_API_KEY) continue; if (!isLoopbackBaseUrl(keys.get("base_url"))) continue; + const modelId = keys.get("model"); + if (!modelId) continue; + let hasOwnershipMarker = hasInlineOwnershipMarker(keys.get("extra_headers")); // Swallow the entry's OWN sub-tables (`[model..extra_headers]`). Grok writes // them when it re-serializes the file, and leaving one behind keeps the alias // reserved by `userModelAliases` — so the sweep would remove the parent and STILL @@ -226,9 +271,22 @@ function findOpencodexOrphans(content: string, region: ManagedRegion | null): Or if (fenceStart >= 0 && header.index < fenceStart && child.index >= fenceStart) break; if (child.segments.length <= 2) break; if (child.segments[0] !== "model" || child.segments[1] !== header.segments[1]) break; - end = clampEnd(header.index, headers[next + 1]?.index ?? content.length); + const childEnd = clampEnd(header.index, headers[next + 1]?.index ?? content.length); + if (!child.array && child.segments.length === 3 && child.segments[2] === "extra_headers") { + const childKeys = tableBodyKeys(content.slice(child.index + child.length, childEnd)); + if (childKeys.get(OPENCODEX_GROK_MARKER) === "1") hasOwnershipMarker = true; + } + end = childEnd; } - orphans.push({ alias: header.segments[1]!, modelId: keys.get("model"), start: header.index, end }); + const legacyGenerated = isLegacyGeneratedTable(header.segments[1]!, keys); + if (!hasOwnershipMarker && !legacyGenerated) continue; + orphans.push({ + alias: header.segments[1]!, + modelId, + ownership: hasOwnershipMarker ? "explicit" : "legacy", + start: header.index, + end, + }); } return orphans; } @@ -352,6 +410,10 @@ export function injectGrokConfig( excluded?: ReadonlySet; /** Unfiltered known ids used only to distinguish hidden current models from retired ones. */ catalogModelIds?: ReadonlySet; + /** Canonical provider keys disabled in config and therefore absent from catalog fetching. */ + disabledProviderNamespaces?: ReadonlySet; + /** Configured combo public ids that may syntactically resemble provider/model ids. */ + comboPublicModelIds?: ReadonlySet; } = {}, ): GrokInjectResult { const grokHome = resolveGrokHome(opts.grokHome); @@ -403,13 +465,25 @@ export function injectGrokConfig( // Adopt our own pre-fence entries (#511) BEFORE reserving user aliases, so the stale // duplicate is replaced instead of routed around forever. Runs inside the normalized // window so the user's dominant EOL is still restored below. - // Use the full UNFILTERED catalog, not the emitted subset: explicitly excluded and otherwise - // hidden current models must still lose stale unfenced tables, or those tables would bypass - // the user's visibility choice. Direct callers that do not have a separate catalog keep the - // historical `models` behavior. + // Durably marked rows use the full UNFILTERED catalog: explicitly excluded and otherwise + // hidden current models must still lose stale generated tables. Ambiguous pre-marker legacy + // rows are migrated only when this write emits their replacement. Direct callers that do not + // have a separate catalog keep the historical `models` behavior. const catalogModelIds = opts.catalogModelIds ?? new Set(models.map(model => model.id)); + const emittedModelIds = new Set(models + .filter(model => !opts.excluded?.has(model.id)) + .map(model => model.id)); const orphans = findOpencodexOrphans(originalContent, originalRegion) - .filter(orphan => orphan.modelId !== undefined && catalogModelIds.has(orphan.modelId)); + .filter(orphan => orphan.ownership === "legacy" + // A legacy fingerprint is not durable deletion authority. Migrate it only when this + // same write will replace the row with a marked managed table. + ? emittedModelIds.has(orphan.modelId) + : catalogModelIds.has(orphan.modelId) + || isDisabledProviderModelId( + orphan.modelId, + opts.disabledProviderNamespaces, + opts.comboPublicModelIds, + )); const content = removeOrphanTables(originalContent, orphans); // Removing bytes above the fence MOVES it: recompute rather than adjust arithmetic, // so the splice below cannot cut the file in the wrong place. @@ -443,7 +517,7 @@ export function injectGrokConfig( } const renames = new Map(); for (const orphan of orphans) { - const replacement = orphan.modelId === undefined ? undefined : survivors.get(orphan.modelId); + const replacement = survivors.get(orphan.modelId); if (replacement && replacement !== orphan.alias) renames.set(orphan.alias, replacement); } nextContent = rewriteAliasReferences(nextContent, renames); @@ -512,8 +586,10 @@ export function stripGrokConfig(opts: { grokHome?: string } = {}): GrokInjectRes else if (restOfFile.length === 0 && prefix.endsWith("\n")) prefix = prefix.slice(0, -1); // Keep the old fence boundary while sweeping. Concatenating first would let the last // pre-fence orphan absorb comment-only or bare-key user content appended after the fence. - const prefixOrphans = findOpencodexOrphans(prefix, null); - const tailOrphans = findOpencodexOrphans(restOfFile, null); + const prefixOrphans = findOpencodexOrphans(prefix, null) + .filter(orphan => orphan.ownership === "explicit"); + const tailOrphans = findOpencodexOrphans(restOfFile, null) + .filter(orphan => orphan.ownership === "explicit"); orphanCount = prefixOrphans.length + tailOrphans.length; stripped = removeOrphanTables(prefix, prefixOrphans) + removeOrphanTables(restOfFile, tailOrphans); @@ -521,7 +597,8 @@ export function stripGrokConfig(opts: { grokHome?: string } = {}): GrokInjectRes // Retired or otherwise non-emitted OpenCodex tables may intentionally remain outside the // fence while the integration is enabled. Teardown owns those strictly identified tables // even after Grok has re-serialized the file and dropped our marker comments. - const orphans = findOpencodexOrphans(content, null); + const orphans = findOpencodexOrphans(content, null) + .filter(orphan => orphan.ownership === "explicit"); if (orphans.length === 0) { return { ok: true, changed: false, message: "No opencodex managed block found in Grok config." }; } diff --git a/src/grok/sync.ts b/src/grok/sync.ts index 3d2957f4f4..561df07dbf 100644 --- a/src/grok/sync.ts +++ b/src/grok/sync.ts @@ -6,9 +6,10 @@ * * Deps are injectable (mirrors src/codex/sync.ts) so tests can run without a live proxy. */ -import { visibleNativeSlugs, filterCatalogVisibleModels, nativeContextLimits, nativeOpenAiContextWindow, nativeOpenAiSlugs, type CatalogModel } from "../codex/catalog"; +import type { CatalogModel } from "../codex/catalog"; import type { OcxConfig } from "../types"; -import { injectGrokConfig, type GrokInjectModel, type GrokInjectResult } from "./inject"; +import { projectGrokCatalog } from "./catalog"; +import { injectGrokConfig, type GrokInjectResult } from "./inject"; export interface GrokSyncDeps { fetchAllModels: (config: OcxConfig) => Promise; @@ -32,28 +33,10 @@ export async function syncGrokConfig( opts: { hostname?: string; grokHome?: string } = {}, deps: GrokSyncDeps = { fetchAllModels: defaultFetchAllModels, injectGrokConfig }, ): Promise { - let models: GrokInjectModel[]; - let catalogModelIds: Set; + let projection: ReturnType; try { const allRouted = await deps.fetchAllModels(config); - const routed = filterCatalogVisibleModels(allRouted, config); - catalogModelIds = new Set([ - ...nativeOpenAiSlugs(), - ...allRouted.map(model => model.alias ?? `${model.provider}/${model.id}`), - ]); - models = [ - // Native slugs carry their context window too. Without it Grok falls back to its own - // default (200k) and understates models like gpt-5.6-sol, which is 372k. This is the same - // accessor the dashboard's native rows use, so the two cannot disagree. - ...visibleNativeSlugs(config).map(id => { - const contextWindow = nativeOpenAiContextWindow(id, nativeContextLimits(config)); - return { id, ...(contextWindow !== undefined ? { contextWindow } : {}) }; - }), - ...routed.map(m => ({ - id: m.alias ?? `${m.provider}/${m.id}`, - ...(m.contextWindow !== undefined ? { contextWindow: m.contextWindow } : {}), - })), - ]; + projection = projectGrokCatalog(allRouted, config); } catch (err) { return { ok: false, @@ -64,10 +47,12 @@ export async function syncGrokConfig( // Pass the FULL list plus the exclusion set: the writer allocates aliases over // everything and emits only what is switched on, so a model's alias never depends on // its neighbours' switches. Absent/empty selection keeps today's behaviour exactly. - return deps.injectGrokConfig(port, models, { + return deps.injectGrokConfig(port, projection.models, { ...(opts.hostname !== undefined ? { hostname: opts.hostname } : {}), ...(opts.grokHome !== undefined ? { grokHome: opts.grokHome } : {}), excluded: new Set(config.grokExcludedModels ?? []), - catalogModelIds, + catalogModelIds: projection.catalogModelIds, + disabledProviderNamespaces: projection.disabledProviderNamespaces, + comboPublicModelIds: projection.comboPublicModelIds, }); } diff --git a/src/server/management/native-integration-routes.ts b/src/server/management/native-integration-routes.ts index 357971a525..af56e8866c 100644 --- a/src/server/management/native-integration-routes.ts +++ b/src/server/management/native-integration-routes.ts @@ -19,11 +19,12 @@ */ import { loadConfig, saveConfigPreservingClaudeCode } from "../../config"; import { readRuntimePort } from "../../config/process-state"; -import { desktopVisibleNativeSlugs, filterCatalogVisibleModels, nativeContextLimits, nativeOpenAiContextWindow, nativeOpenAiSlugs, visibleNativeSlugs } from "../../codex/catalog"; +import { desktopVisibleNativeSlugs, filterCatalogVisibleModels, nativeContextLimits } from "../../codex/catalog"; import { providerContextCap } from "../../providers/context-cap"; import { OPENAI_CODEX_PROVIDER_ID } from "../../providers/openai-tiers"; import { inspectDesktop3pConfigLibrary, removeDesktop3pStandardPivot, writeDesktop3pConfig } from "../../claude/desktop-3p"; -import { injectGrokConfig, stripGrokConfig, type GrokInjectModel } from "../../grok/inject"; +import { projectGrokCatalog } from "../../grok/catalog"; +import { injectGrokConfig, stripGrokConfig } from "../../grok/inject"; import { inspectGrokConfig } from "../../grok/inspect"; import { grokConfigPath } from "../../grok/status"; import { assertNativeTeardownOwned } from "../../integrations/native/ownership-preflight"; @@ -502,27 +503,10 @@ async function handleGrokToggle(ctx: ManagementContext): Promise { * synchronous from entry (012 §One preflight is not enough). */ const fetchModels = deps.fetchAllModels ?? defaultFetchAllModels; - let models: GrokInjectModel[]; - let catalogModelIds: Set; + let projection: ReturnType; try { const allRouted = await fetchModels(config); - const routed = filterCatalogVisibleModels(allRouted, config); - catalogModelIds = new Set([ - ...nativeOpenAiSlugs(), - ...allRouted.map(model => model.alias ?? `${model.provider}/${model.id}`), - ]); - models = [ - // Native slugs carry their context window: without it Grok falls back - // to its own 200k default and understates a 372k model. - ...visibleNativeSlugs(config).map(id => { - const contextWindow = nativeOpenAiContextWindow(id, nativeContextLimits(config)); - return { id, ...(contextWindow !== undefined ? { contextWindow } : {}) }; - }), - ...routed.map(m => ({ - id: m.alias ?? `${m.provider}/${m.id}`, - ...(m.contextWindow !== undefined ? { contextWindow: m.contextWindow } : {}), - })), - ]; + projection = projectGrokCatalog(allRouted, config); } catch (error) { // A catalog failure must never write an empty fence (syncGrokConfig // guards this; the route inherits the rule). Nothing was written. @@ -535,7 +519,7 @@ async function handleGrokToggle(ctx: ManagementContext): Promise { if (recheck.kind === "orphaned_marker") return postCommitRefusal(409, "grok", "orphaned_marker", ORPHANED_MARKER_MESSAGE, { desiredEnabled }); const inject = deps.injectGrokConfig ?? injectGrokConfig; - const result = inject(port, models, { + const result = inject(port, projection.models, { ...(hostname !== undefined ? { hostname } : {}), // The FULL list plus the exclusion set, never a pre-filtered list: the // writer allocates aliases over everything, so a model's alias never @@ -543,7 +527,9 @@ async function handleGrokToggle(ctx: ManagementContext): Promise { excluded: new Set(config.grokExcludedModels ?? []), // Visibility filters decide what to emit, not whether an owned pre-fence table is still // current. Otherwise a hidden model is mistaken for retired state and survives outside. - catalogModelIds, + catalogModelIds: projection.catalogModelIds, + disabledProviderNamespaces: projection.disabledProviderNamespaces, + comboPublicModelIds: projection.comboPublicModelIds, }); if (result.skippedReason === "non-loopback") { diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index b7d7876a5b..635bd5e1b5 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -19,6 +19,7 @@ import { injectGrokConfig, stripGrokConfig } from "../src/grok/inject"; const BEGIN_MARKER = "# >>> opencodex managed block — do not edit (removed by `ocx stop`) >>>"; const MODELS = [{ id: "gpt-5.6-sol", contextWindow: 372_000 }]; +const OWNERSHIP_MARKER = 'extra_headers = { "x-opencodex-grok" = "1" }'; describe("Grok orphan adoption (#511)", () => { let root: string; @@ -45,7 +46,7 @@ describe("Grok orphan adoption (#511)", () => { "[model.ocx-gpt-5-6-sol]", 'model = "gpt-5.6-sol"', 'base_url = "http://127.0.0.1:10100/v1"', - 'api_backend = "responses"', + 'api_backend = "chat_completions"', 'api_key = "opencodex-loopback"', 'name = "OCX gpt-5.6-sol"', "", @@ -110,6 +111,56 @@ describe("Grok orphan adoption (#511)", () => { expect(readFileSync(configPath, "utf8")).toContain("[model.ocx-remote]"); }); + test("preserves documented and generated-looking markerless manual tables", () => { + const fixtures = [ + [ + "[model.ocx-opus]", + 'model = "anthropic/claude-opus-4-8"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "responses"', + 'api_key = "opencodex-loopback"', + "", + ], + [ + "[model.ocx-opus]", + 'model = "anthropic/claude-opus-4-8"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "chat_completions"', + 'api_key = "opencodex-loopback"', + "", + ], + [ + "[model.ocx-anthropic-claude-opus-4-8]", + 'model = "anthropic/claude-opus-4-8"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "responses"', + 'api_key = "opencodex-loopback"', + 'name = "OCX anthropic/claude-opus-4-8"', + "", + ], + [ + "[model.ocx-anthropic-claude-opus-4-8]", + 'model = "anthropic/claude-opus-4-8"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "responses"', + 'api_key = "opencodex-loopback"', + 'name = "OCX anthropic/claude-opus-4-8"', + 'extra_headers = { "x-opencodex-grok" = "0" }', + "", + ], + ]; + + for (const lines of fixtures) { + const original = lines.join("\n"); + writeFileSync(configPath, original); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toContain(original); + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(original); + } + }); + // F3: `[[model.x]]` collides with a generated `[model.x]` and makes Grok reject the // WHOLE config layer, so that spelling must stay reserved rather than adopted. test("leaves an array-of-table model reserved", () => { @@ -157,6 +208,7 @@ describe("Grok orphan adoption (#511)", () => { 'model = "retired/model"', 'base_url = "http://127.0.0.1:10100/v1"', 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, "", ].join("\n")); @@ -173,18 +225,57 @@ describe("Grok orphan adoption (#511)", () => { }); test("keeps an owned-looking orphan whose model id is missing", () => { - writeFileSync(configPath, [ + const original = [ "[model.ocx-unknown]", 'base_url = "http://127.0.0.1:10100/v1"', 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, "", - ].join("\n")); + ].join("\n"); + writeFileSync(configPath, original); const result = injectGrokConfig(10100, MODELS, { grokHome }); expect(result).toMatchObject({ ok: true, changed: true }); const content = readFileSync(configPath, "utf8"); expect(content).toContain("[model.ocx-unknown]"); expect(content).toContain('api_key = "opencodex-loopback"'); + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(original); + }); + + test("preserves empty-model and array-child marker lookalikes", () => { + const fixtures = [ + [ + "[model.ocx-empty]", + 'model = ""', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ], + [ + "[model.ocx-array-marker]", + 'model = "gpt-5.6-sol"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "responses"', + 'api_key = "opencodex-loopback"', + "", + "[[model.ocx-array-marker.extra_headers]]", + 'x-opencodex-grok = "1"', + "", + ], + ]; + + for (const lines of fixtures) { + const original = lines.join("\n"); + writeFileSync(configPath, original); + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: false }); + expect(readFileSync(configPath, "utf8")).toBe(original); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(original); + } }); test("removes a hidden current orphan but preserves a genuinely retired one", () => { @@ -193,11 +284,13 @@ describe("Grok orphan adoption (#511)", () => { 'model = "hidden/model"', 'base_url = "http://127.0.0.1:10100/v1"', 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, "", "[model.ocx-retired]", 'model = "retired/model"', 'base_url = "http://127.0.0.1:10100/v1"', 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, "", ].join("\n")); @@ -236,6 +329,7 @@ describe("Grok orphan adoption (#511)", () => { 'model = "retired/model"', 'base_url = "http://127.0.0.1:10100/v1"', 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, "", ].join(eol); writeFileSync(configPath, userPrefix + orphan); @@ -257,6 +351,7 @@ describe("Grok orphan adoption (#511)", () => { 'model = "retired/model"', 'base_url = "http://127.0.0.1:10100/v1"', 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, "", ].join(eol); const tail = ["# keep this post-fence note", "bare_user_key = true", ""].join(eol); @@ -276,6 +371,7 @@ describe("Grok orphan adoption (#511)", () => { 'model = "retired/model"', 'base_url = "http://127.0.0.1:10100/v1"', 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, "", ].join("\n"); const userOwned = [ @@ -292,8 +388,35 @@ describe("Grok orphan adoption (#511)", () => { expect(readFileSync(configPath, "utf8")).toBe(userOwned); }); + test("markerless teardown preserves an ambiguous legacy row", () => { + const legacy = [ + "[model.ocx-gpt-5-6-sol]", + 'model = "gpt-5.6-sol"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "chat_completions"', + 'api_key = "opencodex-loopback"', + 'name = "OCX gpt-5.6-sol"', + "", + ].join("\n"); + writeFileSync(configPath, legacy); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: false }); + expect(readFileSync(configPath, "utf8")).toBe(legacy); + expect(injectGrokConfig(10100, MODELS, { + grokHome, + excluded: new Set(["gpt-5.6-sol"]), + })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toContain('api_backend = "chat_completions"'); + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(legacy); + // Injection can migrate the same legacy row because it writes a marked replacement now. + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).not.toContain('api_backend = "chat_completions"'); + }); + test("still removes a catalog orphan when that model is excluded", () => { - writeOrphanedConfig(); + writeOrphanedConfig(OWNERSHIP_MARKER); injectGrokConfig(10100, MODELS, { grokHome, @@ -340,7 +463,9 @@ describe("Grok orphan adoption (#511)", () => { "[model.ocx-gpt-5-6-sol]", // stale: no context_window 'model = "gpt-5.6-sol"', 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "chat_completions"', 'api_key = "opencodex-loopback"', + 'name = "OCX gpt-5.6-sol"', "", "[model.ocx-gpt-5-6-sol-2]", // the correct duplicate, also unfenced now 'model = "gpt-5.6-sol"', @@ -477,7 +602,9 @@ describe("Grok orphan adoption — fence boundary (#511 follow-up)", () => { `[model.${alias}]`, 'model = "gpt-5.6-sol"', 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "chat_completions"', 'api_key = "opencodex-loopback"', + 'name = "OCX gpt-5.6-sol"', ]; const fence = (alias: string): string[] => [ @@ -534,7 +661,7 @@ describe("Grok orphan adoption — fence boundary (#511 follow-up)", () => { ...orphan("ocx-gpt-5-6-sol"), "", "[model.ocx-gpt-5-6-sol.extra_headers]", - 'x-opencodex = "1"', + 'x-opencodex-grok = "1"', "", ].join("\n")); diff --git a/tests/grok-sync.test.ts b/tests/grok-sync.test.ts index 177fc15f0c..ada2360aaf 100644 --- a/tests/grok-sync.test.ts +++ b/tests/grok-sync.test.ts @@ -61,7 +61,9 @@ describe("syncGrokConfig", () => { "[model.ocx-stub-hidden]", 'model = "stub/hidden"', 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "responses"', 'api_key = "opencodex-loopback"', + 'extra_headers = { "x-opencodex-grok" = "1" }', "", ].join("\n")); @@ -79,6 +81,83 @@ describe("syncGrokConfig", () => { } }); + test("removes owned orphans from a disabled provider even without fetched ids", async () => { + const { root, grokHome } = tempGrokHome(); + try { + const config = { + ...baseConfig, + providers: { + "disabled-provider": { + adapter: "openai-responses", + baseUrl: "https://example.invalid/v1", + disabled: true, + }, + }, + } as unknown as OcxConfig; + writeFileSync(join(grokHome, "config.toml"), [ + "[model.ocx-disabled-provider-legacy]", + 'model = "disabled-provider/legacy"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "responses"', + 'api_key = "opencodex-loopback"', + 'extra_headers = { "x-opencodex-grok" = "1" }', + "", + ].join("\n")); + + const result = await syncGrokConfig(10190, config, { grokHome }, { + fetchAllModels: async () => [], + injectGrokConfig, + }); + expect(result).toMatchObject({ ok: true, changed: true }); + const content = readFileSync(join(grokHome, "config.toml"), "utf8"); + expect(content).not.toContain("[model.ocx-disabled-provider-legacy]"); + expect(content).not.toContain('model = "disabled-provider/legacy"'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + test("does not reinterpret a slash-shaped combo alias as a disabled provider model", async () => { + const { root, grokHome } = tempGrokHome(); + try { + const config = { + ...baseConfig, + providers: { + "disabled-provider": { + adapter: "openai-responses", + baseUrl: "https://example.invalid/v1", + disabled: true, + }, + }, + combos: { + fallback: { + alias: "disabled-provider/legacy", + targets: [{ provider: "other", model: "m1" }], + }, + }, + } as unknown as OcxConfig; + const manual = [ + "[model.ocx-disabled-provider-legacy]", + 'model = "disabled-provider/legacy"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "responses"', + 'api_key = "opencodex-loopback"', + 'extra_headers = { "x-opencodex-grok" = "1" }', + "", + ].join("\n"); + writeFileSync(join(grokHome, "config.toml"), manual); + + const result = await syncGrokConfig(10190, config, { grokHome }, { + fetchAllModels: async () => [], + injectGrokConfig, + }); + expect(result).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(join(grokHome, "config.toml"), "utf8")).toContain(manual); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + test("keeps disabled native ids in the orphan-classification catalog", async () => { const hiddenNative = nativeOpenAiSlugs()[0]!; let emitted: GrokInjectModel[] | undefined; diff --git a/tests/native-grok-toggle.test.ts b/tests/native-grok-toggle.test.ts index 5fa24382f9..ccd5c864af 100644 --- a/tests/native-grok-toggle.test.ts +++ b/tests/native-grok-toggle.test.ts @@ -347,6 +347,20 @@ test("the route never calls syncGrokConfig, and the inspector never re-implement test("the route's model list is byte-identical to syncGrokConfig's", async () => { writeConfig("# user only\n"); const config = baseConfig({ + providers: { + stub: { adapter: "openai-responses", baseUrl: "https://example.invalid/v1" }, + "disabled-stub": { + adapter: "openai-responses", + baseUrl: "https://example.invalid/v1", + disabled: true, + }, + }, + combos: { + slashy: { + alias: "disabled-stub/m4", + targets: [{ provider: "stub", model: "m1" }], + }, + }, disabledModels: ["stub/m2"], grokExcludedModels: ["stub/m3"], }); @@ -358,12 +372,16 @@ test("the route's model list is byte-identical to syncGrokConfig's", async () => let routeModels: GrokInjectModel[] | null = null; let routeExcluded: ReadonlySet | null = null; let routeCatalogModelIds: ReadonlySet | null = null; + let routeDisabledProviderNamespaces: ReadonlySet | null = null; + let routeComboPublicModelIds: ReadonlySet | null = null; const routeDeps = testDeps({ fetchAllModels: (async () => catalog) as never, injectGrokConfig: ((port: number, models: GrokInjectModel[], opts: Parameters[2]) => { routeModels = models; routeExcluded = opts?.excluded ?? null; routeCatalogModelIds = opts?.catalogModelIds ?? null; + routeDisabledProviderNamespaces = opts?.disabledProviderNamespaces ?? null; + routeComboPublicModelIds = opts?.comboPublicModelIds ?? null; return injectGrokConfig(port, models, opts); }) as typeof injectGrokConfig, }); @@ -373,12 +391,16 @@ test("the route's model list is byte-identical to syncGrokConfig's", async () => let syncModels: GrokInjectModel[] | null = null; let syncExcluded: ReadonlySet | null = null; let syncCatalogModelIds: ReadonlySet | null = null; + let syncDisabledProviderNamespaces: ReadonlySet | null = null; + let syncComboPublicModelIds: ReadonlySet | null = null; await syncGrokConfig(10100, config, { hostname: "127.0.0.1" }, { fetchAllModels: (async () => catalog) as never, injectGrokConfig: ((port: number, models: GrokInjectModel[], opts: Parameters[2]) => { syncModels = models; syncExcluded = opts?.excluded ?? null; syncCatalogModelIds = opts?.catalogModelIds ?? null; + syncDisabledProviderNamespaces = opts?.disabledProviderNamespaces ?? null; + syncComboPublicModelIds = opts?.comboPublicModelIds ?? null; return injectGrokConfig(port, models, opts); }) as typeof injectGrokConfig, }); @@ -386,6 +408,12 @@ test("the route's model list is byte-identical to syncGrokConfig's", async () => expect(routeCatalogModelIds && [...routeCatalogModelIds].sort()) .toEqual(syncCatalogModelIds && [...syncCatalogModelIds].sort()); expect(routeCatalogModelIds?.has("stub/m2")).toBe(true); + expect(routeDisabledProviderNamespaces && [...routeDisabledProviderNamespaces].sort()) + .toEqual(syncDisabledProviderNamespaces && [...syncDisabledProviderNamespaces].sort()); + expect(routeDisabledProviderNamespaces?.has("disabled-stub")).toBe(true); + expect(routeComboPublicModelIds && [...routeComboPublicModelIds].sort()) + .toEqual(syncComboPublicModelIds && [...syncComboPublicModelIds].sort()); + expect(routeComboPublicModelIds?.has("disabled-stub/m4")).toBe(true); /* * The exclusion half of the clause (C-gate blocker): the FULL list goes to * the writer together with the exclusion SET, never a pre-filtered list — From 57b5eefa1a2fa38da2bfd04b0a99e2e724278810 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 02:32:15 +0900 Subject: [PATCH 4/8] fix(grok): make orphan cleanup TOML-safe (cherry picked from commit 387d9f2b1035afa16af36ba7acd020555859a756) --- src/grok/inject.ts | 589 +++++++++++++++++++++++++---- tests/grok-orphan-adoption.test.ts | 457 +++++++++++++++++++++- 2 files changed, 969 insertions(+), 77 deletions(-) diff --git a/src/grok/inject.ts b/src/grok/inject.ts index dc407d8109..7d1478811f 100644 --- a/src/grok/inject.ts +++ b/src/grok/inject.ts @@ -71,30 +71,283 @@ export function findManagedRegion(content: string): ManagedRegion | null { * `[model.]` header must be canonicalized before comparison. */ const KEY_SEGMENT = String.raw`(?:[A-Za-z0-9_-]+|"(?:[^"\\]|\\.)*"|'[^']*')`; +const DOTTED_KEY = String.raw`${KEY_SEGMENT}(?:[ \t]*\.[ \t]*${KEY_SEGMENT})*`; +/** One complete TOML table-header line; paired brackets reject array-value lookalikes. */ +const TABLE_HEADER_LINE = new RegExp( + String.raw`^[ \t]*(?:\[\[[ \t]*(${DOTTED_KEY})[ \t]*\]\]|\[[ \t]*(${DOTTED_KEY})[ \t]*\])[ \t]*(?:#[^\r\n]*)?$`, +); + +interface TomlTableHeader { + index: number; + length: number; + segments: string[]; + array: boolean; +} + +interface TomlStructure { + view: string; + headers: TomlTableHeader[]; + containerRootLineStarts: Set; +} + +/** End of a TOML multi-line basic/literal string, or EOF when it is unclosed. */ +function tomlMultilineStringEnd(content: string, start: number, quote: '"' | "'"): number { + let cursor = start + 3; + while (cursor < content.length) { + if (quote === '"' && content[cursor] === "\\") { + cursor += 2; + continue; + } + if (content[cursor] === quote + && content[cursor + 1] === quote + && content[cursor + 2] === quote) { + let end = cursor + 3; + // TOML permits one or two quote characters immediately before the closing delimiter. + if (content[end] === quote) { + end += 1; + if (content[end] === quote) end += 1; + } + return end; + } + cursor += 1; + } + return content.length; +} + +/** Find one TOML string value's exact source span; semantic decoding uses Bun's parser. */ +function tomlStringSpanAt(content: string, start: number): { end: number } | null { + const quote = content[start]; + if (quote !== '"' && quote !== "'") return null; + if (content[start + 1] === quote && content[start + 2] === quote) { + const end = tomlMultilineStringEnd(content, start, quote); + const token = content.slice(start, end); + if (token.length < 6 || !token.endsWith(quote.repeat(3))) return null; + return { end }; + } + + for (let cursor = start + 1; cursor < content.length; cursor += 1) { + const char = content[cursor]!; + if (char === "\r" || char === "\n") return null; + if (quote === '"' && char === "\\") { + cursor += 1; + continue; + } + if (char === quote) { + return { end: cursor + 1 }; + } + } + return null; +} + +/** Find the matching end of one inline table / array while skipping strings and comments. */ +function tomlContainerEnd(content: string, start: number): number | null { + const opener = content[start]; + if (opener !== "{" && opener !== "[") return null; + const stack: string[] = [opener]; + for (let index = start + 1; index < content.length;) { + const char = content[index]!; + if (char === "#") { + const newline = content.indexOf("\n", index); + index = newline === -1 ? content.length : newline + 1; + continue; + } + if (char === '"' || char === "'") { + const span = tomlStringSpanAt(content, index); + if (span === null) return null; + index = span.end; + continue; + } + if (char === "{" || char === "[") stack.push(char); + else if (char === "}" || char === "]") { + const expected = char === "}" ? "{" : "["; + if (stack.pop() !== expected) return null; + if (stack.length === 0) return index + 1; + } + index += 1; + } + return null; +} + +/** Locate one top-level string member inside an inline table without scanning nested prose. */ +function tomlInlineStringValueSpan( + content: string, + start: number, + end: number, + targetKey: string, +): { start: number; end: number } | null { + const skipTrivia = (from: number): number => { + let cursor = from; + while (cursor < end) { + while (/[ \t\r\n]/.test(content[cursor] ?? "")) cursor += 1; + if (content[cursor] !== "#") break; + const newline = content.indexOf("\n", cursor); + cursor = newline === -1 || newline >= end ? end : newline + 1; + } + return cursor; + }; + const keyPattern = new RegExp(DOTTED_KEY, "y"); + let entryStart = start + 1; + while (entryStart < end - 1) { + entryStart = skipTrivia(entryStart); + if (entryStart >= end - 1 || content[entryStart] === "}") return null; + keyPattern.lastIndex = entryStart; + const key = keyPattern.exec(content); + if (key === null) return null; + let cursor = skipTrivia(keyPattern.lastIndex); + if (content[cursor] !== "=") return null; + const valueStart = skipTrivia(cursor + 1); + const segments = canonicalDottedKey(key[0]); + if (segments.length === 1 && segments[0] === targetKey) { + const value = tomlStringSpanAt(content, valueStart); + return value === null ? null : { start: valueStart, end: value.end }; + } + + const stack: string[] = []; + cursor = valueStart; + let foundNext = false; + while (cursor < end - 1) { + const char = content[cursor]!; + if (char === "#") { + const newline = content.indexOf("\n", cursor); + cursor = newline === -1 || newline >= end ? end : newline + 1; + continue; + } + if (char === '"' || char === "'") { + const value = tomlStringSpanAt(content, cursor); + if (value === null) return null; + cursor = value.end; + continue; + } + if (char === "{" || char === "[") stack.push(char); + else if (char === "}" || char === "]") { + if (stack.length === 0) return null; + const expected = char === "}" ? "{" : "["; + if (stack.pop() !== expected) return null; + } else if (char === "," && stack.length === 0) { + entryStart = cursor + 1; + foundNext = true; + break; + } + cursor += 1; + } + if (!foundNext) return null; + } + return null; +} + /** - * User-owned model table headers. Also matches array-of-table (`[[model.x]]`) and sub-table - * (`[model.x.sub]`) spellings. `[[model.x]]` genuinely collides with a generated `[model.x]`, - * and one collision makes grok reject the ENTIRE config layer ("duplicate key"), taking every - * unrelated user setting with it; `[model.x.sub]` does not strictly collide, but reserving it - * costs only a suffixed alias and keeps us clear of the user's namespace. - * - * Every character class here is newline-free ON PURPOSE. With `[^\]]*` the optional sub-table - * tail runs past the end of its own line, so an unclosed `[model.…` inside a multiline string - * swallows the following lines — including a real `[model.]` header, which then goes - * unreserved and produces the very duplicate-key config this scan exists to prevent. + * A same-length lexical projection for structural scans. Triple-quoted string bytes become + * spaces while line endings and every byte outside those values keep their original offsets. */ -const MODEL_TABLE_HEADER = new RegExp( - String.raw`^[ \t]*\[\[?[ \t]*(${KEY_SEGMENT})[ \t]*\.[ \t]*(${KEY_SEGMENT})[ \t]*(?:\.[^\]\r\n]*)?\]\]?[ \t]*(?:#.*)?$`, - "gm", -); +function tomlStructuralView(content: string): string { + let state: "code" | "comment" | "basic" | "literal" = "code"; + let cursor = 0; + let output = ""; + for (let index = 0; index < content.length;) { + const char = content[index]!; + if (state === "comment") { + if (char === "\n") state = "code"; + index += 1; + continue; + } + if (state === "basic") { + if (char === "\\") index += 2; + else { + if (char === '"') state = "code"; + index += 1; + } + continue; + } + if (state === "literal") { + if (char === "'") state = "code"; + index += 1; + continue; + } + if (char === "#") { + state = "comment"; + index += 1; + continue; + } + if (char === '"' || char === "'") { + if (content[index + 1] === char && content[index + 2] === char) { + const end = tomlMultilineStringEnd(content, index, char); + output += content.slice(cursor, index); + output += content.slice(index, end).replace(/[^\r\n]/g, " "); + cursor = end; + index = end; + continue; + } + state = char === '"' ? "basic" : "literal"; + } + index += 1; + } + return output.length === 0 ? content : output + content.slice(cursor); +} + +/** Update array / inline-table nesting for one non-header line in the structural view. */ +function tomlContainerDepthAfterLine(line: string, initialDepth: number): number { + let depth = initialDepth; + let state: "code" | "basic" | "literal" = "code"; + for (let index = 0; index < line.length;) { + const char = line[index]!; + if (state === "basic") { + if (char === "\\") index += 2; + else { + if (char === '"') state = "code"; + index += 1; + } + continue; + } + if (state === "literal") { + if (char === "'") state = "code"; + index += 1; + continue; + } + if (char === "#") break; + if (char === '"' || char === "'") { + state = char === '"' ? "basic" : "literal"; + index += 1; + continue; + } + if (char === "[" || char === "{") depth += 1; + else if (char === "]" || char === "}") depth = Math.max(0, depth - 1); + index += 1; + } + return depth; +} /** - * ANY table header, capturing its full dotted key. Used to compute table SPANS: a table - * body runs from its own header to the next header of any kind, so the orphan sweep can - * remove a whole table instead of a guessed line range (a partial removal would re-parent - * the leftover keys onto the preceding table). + * Find real table headers and assignment-eligible lines while excluding arrays, inline tables, + * comments, and multi-line strings. Offsets remain exact because `view` is length-preserving. */ -const ANY_TABLE_HEADER = /^[ \t]*\[\[?[ \t]*([^\]\r\n]*?)[ \t]*\]\]?[ \t]*(?:#.*)?$/gm; +function analyzeTomlStructure(content: string): TomlStructure { + const view = tomlStructuralView(content); + const headers: TomlTableHeader[] = []; + const containerRootLineStarts = new Set(); + let depth = 0; + for (let lineStart = 0; lineStart <= view.length;) { + const newline = view.indexOf("\n", lineStart); + const lineEnd = newline === -1 ? view.length : newline; + const rawLine = view.slice(lineStart, lineEnd); + const line = rawLine.endsWith("\r") ? rawLine.slice(0, -1) : rawLine; + const header = depth === 0 ? TABLE_HEADER_LINE.exec(line) : null; + if (header) { + const dottedKey = header[1] ?? header[2]!; + headers.push({ + index: lineStart, + length: header[0].length, + segments: canonicalDottedKey(dottedKey), + array: header[1] !== undefined, + }); + } else { + if (depth === 0) containerRootLineStarts.add(lineStart); + depth = tomlContainerDepthAfterLine(line, depth); + } + if (newline === -1) break; + lineStart = newline + 1; + } + return { view, headers, containerRootLineStarts }; +} /** Resolve a header key segment (bare / basic / literal) to the key it actually addresses. */ function canonicalKeySegment(raw: string): string { @@ -103,6 +356,12 @@ function canonicalKeySegment(raw: string): string { return raw; } +/** Split a TOML dotted key without treating dots inside quoted segments as separators. */ +function canonicalDottedKey(raw: string): string[] { + return [...raw.matchAll(new RegExp(KEY_SEGMENT, "g"))] + .map(match => canonicalKeySegment(match[0]!)); +} + /** * `[model.]` table headers the USER owns (outside our fence) — reserved for collisions. * TOML admits equivalent header spellings for BOTH segments (`["model"."ocx-mine"]`, @@ -114,9 +373,9 @@ function userModelAliases(content: string, region: ManagedRegion | null): Set(); - for (const match of outsideManagedRegion.matchAll(MODEL_TABLE_HEADER)) { - if (canonicalKeySegment(match[1]!) !== "model") continue; - aliases.add(canonicalKeySegment(match[2]!)); + for (const header of analyzeTomlStructure(outsideManagedRegion).headers) { + if (header.segments[0] !== "model" || header.segments.length < 2) continue; + aliases.add(header.segments[1]!); } return aliases; } @@ -135,14 +394,17 @@ interface OrphanTable { /** Offsets into the NORMALIZED content: header start .. next header start (or EOF). */ start: number; end: number; + /** Re-serialized child tables may be separated from the parent by unrelated tables. */ + additionalRanges: Array<{ start: number; end: number }>; } /** `key = "value"` / `key = value` pairs at the top level of one table body. */ function tableBodyKeys(body: string): Map { const keys = new Map(); - for (const line of body.split("\n")) { - const match = /^[ \t]*([A-Za-z0-9_-]+)[ \t]*=[ \t]*(.*?)[ \t]*$/.exec(line); - if (!match) continue; + const structure = analyzeTomlStructure(body); + const assignment = /^[ \t]*([A-Za-z0-9_-]+)[ \t]*=[ \t]*(.*?)[ \t]*$/gm; + for (const match of structure.view.matchAll(assignment)) { + if (!structure.containerRootLineStarts.has(match.index!)) continue; const raw = match[2]!; const value = raw.length >= 2 && raw.startsWith('"') && raw.endsWith('"') ? decodeTomlBasicString(raw.slice(1, -1)) @@ -238,15 +500,7 @@ function findOpencodexOrphans(content: string, region: ManagedRegion | null): Or const clampEnd = (start: number, end: number): number => fenceStart >= 0 && start < fenceStart ? Math.min(end, fenceStart) : end; // Collect every table header first: a table body runs to the NEXT header, whatever it is. - const headers: Array<{ index: number; length: number; segments: string[]; array: boolean }> = []; - for (const match of content.matchAll(ANY_TABLE_HEADER)) { - headers.push({ - index: match.index!, - length: match[0].length, - segments: match[1]!.split(".").map(part => canonicalKeySegment(part.trim())), - array: match[0].trimStart().startsWith("[["), - }); - } + const headers = analyzeTomlStructure(content).headers; for (const [position, header] of headers.entries()) { if (header.array || header.segments.length !== 2 || header.segments[0] !== "model") continue; // Inside the fence the regular splice already owns it. @@ -258,25 +512,23 @@ function findOpencodexOrphans(content: string, region: ManagedRegion | null): Or const modelId = keys.get("model"); if (!modelId) continue; let hasOwnershipMarker = hasInlineOwnershipMarker(keys.get("extra_headers")); - // Swallow the entry's OWN sub-tables (`[model..extra_headers]`). Grok writes - // them when it re-serializes the file, and leaving one behind keeps the alias - // reserved by `userModelAliases` — so the sweep would remove the parent and STILL - // allocate a suffixed duplicate, which is the exact #511 loop we came to close. - let end = bodyEnd; - for (let next = position + 1; next < headers.length; next += 1) { + // Swallow the entry's OWN sub-tables (`[model..extra_headers]`). Grok may + // re-serialize them non-contiguously, so collect exact descendant spans globally rather + // than stopping at the first unrelated table. + const additionalRanges: Array<{ start: number; end: number }> = []; + for (let next = 0; next < headers.length; next += 1) { + if (next === position) continue; const child = headers[next]!; - // Only a PRE-fence parent may be cut short by the fence. Without the parent test a - // below-fence orphan would break on its first child (every index is past the fence), - // leaving the sub-table behind to keep the alias reserved — the -2 loop again. - if (fenceStart >= 0 && header.index < fenceStart && child.index >= fenceStart) break; - if (child.segments.length <= 2) break; - if (child.segments[0] !== "model" || child.segments[1] !== header.segments[1]) break; - const childEnd = clampEnd(header.index, headers[next + 1]?.index ?? content.length); + if (region && child.index >= region.start && child.index < region.end) continue; + if (child.segments.length <= 2 + || child.segments[0] !== "model" + || child.segments[1] !== header.segments[1]) continue; + const childEnd = clampEnd(child.index, headers[next + 1]?.index ?? content.length); + additionalRanges.push({ start: child.index, end: childEnd }); if (!child.array && child.segments.length === 3 && child.segments[2] === "extra_headers") { const childKeys = tableBodyKeys(content.slice(child.index + child.length, childEnd)); if (childKeys.get(OPENCODEX_GROK_MARKER) === "1") hasOwnershipMarker = true; } - end = childEnd; } const legacyGenerated = isLegacyGeneratedTable(header.segments[1]!, keys); if (!hasOwnershipMarker && !legacyGenerated) continue; @@ -285,36 +537,197 @@ function findOpencodexOrphans(content: string, region: ManagedRegion | null): Or modelId, ownership: hasOwnershipMarker ? "explicit" : "legacy", start: header.index, - end, + end: bodyEnd, + additionalRanges, }); } return orphans; } -/** Remove whole tables, back to front so earlier offsets stay valid. */ +function orphanRanges(orphans: readonly OrphanTable[]): Array<{ start: number; end: number }> { + const unique = new Map(); + for (const orphan of orphans) { + for (const range of [{ start: orphan.start, end: orphan.end }, ...orphan.additionalRanges]) { + unique.set(`${range.start}:${range.end}`, range); + } + } + return [...unique.values()]; +} + +/** Remove exact whole-table ranges, back to front so earlier offsets stay valid. */ +function removeTableRanges(content: string, ranges: readonly { start: number; end: number }[]): string { + let next = content; + const unique = new Map(ranges.map(range => [`${range.start}:${range.end}`, range])); + for (const range of [...unique.values()].sort((a, b) => b.start - a.start)) { + next = next.slice(0, range.start) + next.slice(range.end); + } + return next; +} + function removeOrphanTables(content: string, orphans: OrphanTable[]): string { + return removeTableRanges(content, orphanRanges(orphans)); +} + +/** Read one exact path from an already parsed TOML document. */ +function tomlPathString(document: unknown, path: readonly string[]): string | null { + let value = document; + for (const segment of path) { + if (typeof value !== "object" || value === null || Array.isArray(value)) return null; + value = (value as Record)[segment]; + } + return typeof value === "string" ? value : null; +} + +/** Parse a probe document and read one exact semantic path. */ +function parsedTomlPathString(content: string, path: readonly string[]): string | null { + try { + return tomlPathString(Bun.TOML.parse(content), path); + } catch { + return null; + } +} + +/** Rename or remove the two semantic model references without touching user prose. */ +function transformAliasReferences( + content: string, + replacements: ReadonlyMap, + allowRootDotted = true, +): string { + if (replacements.size === 0) return content; + let document: unknown; + try { + document = Bun.TOML.parse(content); + } catch { + throw new Error( + "Grok config rewrite refused: Bun could not parse the TOML document safely.", + ); + } + const structure = analyzeTomlStructure(content); + const edits: Array<{ start: number; end: number; replacement: string }> = []; + const candidates: Array<{ + pathKey: "models.default" | "ui.fork_secondary_model"; + valueStart: number; + valueEnd: number; + line: { start: number; end: number } | null; + }> = []; + const assignment = new RegExp(String.raw`^([ \t]*(${DOTTED_KEY})[ \t]*=)`, "gm"); + let headerPosition = -1; + for (const match of structure.view.matchAll(assignment)) { + const assignmentStart = match.index!; + if (!structure.containerRootLineStarts.has(assignmentStart)) continue; + while ((structure.headers[headerPosition + 1]?.index ?? Number.POSITIVE_INFINITY) + < assignmentStart) headerPosition += 1; + const currentHeader = headerPosition >= 0 ? structure.headers[headerPosition]! : null; + if (currentHeader?.array) continue; + if (!allowRootDotted && currentHeader === null) continue; + const segments = canonicalDottedKey(match[2]!); + const semanticPath = [...(currentHeader?.segments ?? []), ...segments]; + let valueStart = assignmentStart + match[1]!.length; + while (content[valueStart] === " " || content[valueStart] === "\t") valueStart += 1; + const pathKey = semanticPath.length === 2 && semanticPath[0] === "models" + && semanticPath[1] === "default" + ? "models.default" + : semanticPath.length === 2 && semanticPath[0] === "ui" + && semanticPath[1] === "fork_secondary_model" + ? "ui.fork_secondary_model" + : null; + if (pathKey !== null) { + const value = tomlStringSpanAt(content, valueStart); + if (value === null) continue; + const suffix = /^[ \t]*(?:#[^\r\n]*)?(?:\r?\n|$)/.exec(content.slice(value.end)); + if (suffix === null) continue; + candidates.push({ + pathKey, + valueStart, + valueEnd: value.end, + line: { start: assignmentStart, end: value.end + suffix[0].length }, + }); + continue; + } + + // The only non-line form we support is a root inline table (`models = { ... }` / `ui =`). + const inlineTarget = currentHeader === null && semanticPath.length === 1 + && semanticPath[0] === "models" + ? { pathKey: "models.default" as const, key: "default" } + : currentHeader === null && semanticPath.length === 1 && semanticPath[0] === "ui" + ? { pathKey: "ui.fork_secondary_model" as const, key: "fork_secondary_model" } + : null; + if (inlineTarget === null || content[valueStart] !== "{") continue; + const inlineEnd = tomlContainerEnd(content, valueStart); + if (inlineEnd === null) continue; + const value = tomlInlineStringValueSpan(content, valueStart, inlineEnd, inlineTarget.key); + if (value === null) continue; + candidates.push({ + pathKey: inlineTarget.pathKey, + valueStart: value.start, + valueEnd: value.end, + line: null, + }); + } + + const targets = [ + { path: ["models", "default"] as const, pathKey: "models.default" as const }, + { path: ["ui", "fork_secondary_model"] as const, pathKey: "ui.fork_secondary_model" as const }, + ]; + for (const [targetIndex, target] of targets.entries()) { + const currentAlias = tomlPathString(document, target.path); + if (currentAlias === null || !replacements.has(currentAlias)) continue; + const replacement = replacements.get(currentAlias)!; + const probeCandidates = candidates.filter(candidate => candidate.pathKey === target.pathKey); + if (probeCandidates.length === 0 && !allowRootDotted) continue; + if (probeCandidates.length === 0 || probeCandidates.length > 32) { + throw new Error( + "Grok config rewrite refused: the model-reference source could not be bounded safely.", + ); + } + let located = false; + for (const candidate of probeCandidates) { + let sentinel = `__opencodex_reference_probe_${targetIndex}_${candidate.valueStart}__`; + while (sentinel === currentAlias) sentinel += "_"; + const probe = content.slice(0, candidate.valueStart) + + tomlString(sentinel) + + content.slice(candidate.valueEnd); + if (parsedTomlPathString(probe, target.path) !== sentinel) continue; + if (replacement === null && candidate.line === null) { + throw new Error( + "Grok teardown refused: a model reference uses an inline TOML shape that cannot " + + "be removed without rewriting user-owned bytes.", + ); + } + edits.push(replacement === null + ? { start: candidate.line!.start, end: candidate.line!.end, replacement: "" } + : { start: candidate.valueStart, end: candidate.valueEnd, replacement: tomlString(replacement) }); + located = true; + break; + } + if (!located) { + throw new Error( + "Grok config rewrite refused: the semantic model reference could not be located safely.", + ); + } + } let next = content; - for (const orphan of [...orphans].sort((a, b) => b.start - a.start)) { - next = next.slice(0, orphan.start) + next.slice(orphan.end); + for (const edit of edits.sort((a, b) => b.start - a.start)) { + next = next.slice(0, edit.start) + edit.replacement + next.slice(edit.end); } return next; } -/** - * Repoint `default` / `fork_secondary_model` at the alias that survived. - * - * Removing an adopted orphan that `[models] default` names would leave Grok pointing at - * a model that no longer exists — and on a real machine `default` DOES name one, so this - * is the common path rather than an edge case. - */ +/** Repoint references at whichever alias survived orphan adoption. */ function rewriteAliasReferences(content: string, renames: Map): string { - if (renames.size === 0) return content; - return content.replace( - /^([ \t]*(?:default|fork_secondary_model)[ \t]*=[ \t]*")([^"]*)(")/gm, - (whole, prefix: string, value: string, suffix: string) => { - const replacement = renames.get(value); - return replacement ? `${prefix}${replacement}${suffix}` : whole; - }, + return transformAliasReferences(content, renames); +} + +/** Remove only references that name model aliases teardown actually swept. */ +function removeAliasReferences( + content: string, + removedAliases: ReadonlySet, + allowRootDotted = true, +): string { + return transformAliasReferences( + content, + new Map([...removedAliases].map(alias => [alias, null] as const)), + allowRootDotted, ); } @@ -508,11 +921,14 @@ export function injectGrokConfig( // file beats a dangling one. if (orphans.length > 0) { const survivors = new Map(); - for (const match of nextContent.matchAll(MODEL_TABLE_HEADER)) { - if (canonicalKeySegment(match[1]!) !== "model") continue; - const alias = canonicalKeySegment(match[2]!); - const body = nextContent.slice(match.index! + match[0].length); - const modelId = tableBodyKeys(body.slice(0, body.search(/^[ \t]*\[/m) + 1 || body.length)).get("model"); + const structure = analyzeTomlStructure(nextContent); + const managedRegion = findManagedRegion(nextContent); + for (const [position, header] of structure.headers.entries()) { + if (header.array || header.segments.length !== 2 || header.segments[0] !== "model") continue; + if (!managedRegion || header.index < managedRegion.start || header.index >= managedRegion.end) continue; + const alias = header.segments[1]!; + const bodyEnd = structure.headers[position + 1]?.index ?? nextContent.length; + const modelId = tableBodyKeys(nextContent.slice(header.index + header.length, bodyEnd)).get("model"); if (modelId !== undefined && !survivors.has(modelId)) survivors.set(modelId, alias); } const renames = new Map(); @@ -574,6 +990,8 @@ export function stripGrokConfig(opts: { grokHome?: string } = {}): GrokInjectRes let stripped: string; let orphanCount = 0; if (originalRegion) { + const fullOrphans = findOpencodexOrphans(content, originalRegion) + .filter(orphan => orphan.ownership === "explicit"); let removalEnd = originalRegion.end; if (content.startsWith("\n", removalEnd)) removalEnd += 1; let prefix = content.slice(0, originalRegion.start); @@ -590,9 +1008,31 @@ export function stripGrokConfig(opts: { grokHome?: string } = {}): GrokInjectRes .filter(orphan => orphan.ownership === "explicit"); const tailOrphans = findOpencodexOrphans(restOfFile, null) .filter(orphan => orphan.ownership === "explicit"); - orphanCount = prefixOrphans.length + tailOrphans.length; - stripped = removeOrphanTables(prefix, prefixOrphans) - + removeOrphanTables(restOfFile, tailOrphans); + const removedAliases = new Set( + [...fullOrphans, ...prefixOrphans, ...tailOrphans].map(orphan => orphan.alias), + ); + orphanCount = removedAliases.size; + const fullRanges = orphanRanges(fullOrphans); + const prefixRanges = [ + ...orphanRanges(prefixOrphans), + ...fullRanges.filter(range => range.end <= originalRegion.start), + ]; + const tailRanges = [ + ...orphanRanges(tailOrphans), + ...fullRanges + .filter(range => range.start >= removalEnd) + .map(range => ({ start: range.start - removalEnd, end: range.end - removalEnd })), + ]; + // Preserve the original fence as a structural boundary while cleaning references too. + // Joining first can re-parent a headerless tail under the last table in `prefix`. + stripped = removeAliasReferences( + removeTableRanges(prefix, prefixRanges), + removedAliases, + ) + removeAliasReferences( + removeTableRanges(restOfFile, tailRanges), + removedAliases, + false, + ); } else { // Retired or otherwise non-emitted OpenCodex tables may intentionally remain outside the // fence while the integration is enabled. Teardown owns those strictly identified tables @@ -604,6 +1044,7 @@ export function stripGrokConfig(opts: { grokHome?: string } = {}): GrokInjectRes } orphanCount = orphans.length; stripped = removeOrphanTables(content, orphans); + stripped = removeAliasReferences(stripped, new Set(orphans.map(orphan => orphan.alias))); } if (orphanCount > 0) copyBackupOnce(configPath, join(grokHome, "config.toml.bak-opencodex")); atomicWriteFile(configPath, applyEol(stripped, eol)); diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index 635bd5e1b5..2b36fd0956 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -343,9 +343,14 @@ describe("Grok orphan adoption (#511)", () => { } }); - test("teardown preserves a comment-only tail beyond the fence byte-for-byte", () => { + test("teardown preserves a headerless tail beyond the fence byte-for-byte", () => { for (const eol of ["\n", "\r\n"]) { - const userPrefix = [`theme = "${eol === "\n" ? "lf" : "crlf"}"`, "", ""].join(eol); + const userPrefix = [ + "[models]", + `keep = "${eol === "\n" ? "lf" : "crlf"}"`, + "", + "", + ].join(eol); const orphan = [ "[model.ocx-retired]", 'model = "retired/model"', @@ -354,7 +359,13 @@ describe("Grok orphan adoption (#511)", () => { OWNERSHIP_MARKER, "", ].join(eol); - const tail = ["# keep this post-fence note", "bare_user_key = true", ""].join(eol); + const tail = [ + "# keep this post-fence note", + 'default = "ocx-retired"', + 'models.default = "ocx-retired"', + "bare_user_key = true", + "", + ].join(eol); writeFileSync(configPath, userPrefix + orphan); expect(injectGrokConfig(10100, MODELS, { grokHome })) .toMatchObject({ ok: true, changed: true }); @@ -388,6 +399,446 @@ describe("Grok orphan adoption (#511)", () => { expect(readFileSync(configPath, "utf8")).toBe(userOwned); }); + test("teardown clears only section-owned references to swept aliases", () => { + for (const withFence of [false, true]) { + const modelsHeader = withFence ? '["models"]' : "[models]"; + const defaultKey = withFence ? '"default"' : "default"; + const uiHeader = withFence ? "['ui']" : "[ui]"; + const secondaryKey = withFence ? "'fork_secondary_model'" : "fork_secondary_model"; + const otherHeader = withFence ? '["other]"]' : "[other]"; + const expected = [ + modelsHeader, + 'keep = "models"', + "", + uiHeader, + 'keep = "ui"', + "", + otherHeader, + 'default = "ocx-retired"', + 'fork_secondary_model = "ocx-retired"', + "", + "", + ].join("\n"); + writeFileSync(configPath, [ + modelsHeader, + `${defaultKey} = "ocx-retired"`, + 'keep = "models"', + "", + uiHeader, + `${secondaryKey} = 'ocx-retired' # removed with its table`, + 'keep = "ui"', + "", + otherHeader, + 'default = "ocx-retired"', + 'fork_secondary_model = "ocx-retired"', + "", + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + if (withFence) { + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + } + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(expected); + } + }); + + test("teardown clears multiline section-owned references to swept aliases", () => { + for (const delimiter of ['"""', "'''"]) { + const original = [ + "[models]", + `default = ${delimiter}ocx-retired${delimiter}`, + 'keep = "models"', + "", + "[ui]", + `fork_secondary_model = ${delimiter}`, + "ocx-retired" + delimiter, + 'keep = "ui"', + "", + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n"); + writeFileSync(configPath, original); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + expect(content).toBe([ + "[models]", + 'keep = "models"', + "", + "[ui]", + 'keep = "ui"', + "", + "", + ].join("\n")); + } + }); + + test("teardown preserves an escaped multiline value that is not the swept alias", () => { + const reference = [ + "[models]", + 'default = """\\\\', + 'u006Fcx-retired"""', + 'keep = "models"', + "", + ].join("\n"); + writeFileSync(configPath, reference + [ + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(reference); + }); + + test("teardown does not reinterpret nested array elements as table headers", () => { + const userContent = [ + "[other]", + "model_names = [", + ' ["models"],', + "]", + 'default = "ocx-retired"', + "ui_names = [", + ' ["ui"],', + "]", + 'fork_secondary_model = "ocx-retired"', + "", + "", + ].join("\n"); + writeFileSync(configPath, userContent + [ + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(userContent); + }); + + test("teardown clears quoted root dotted references to swept aliases", () => { + writeFileSync(configPath, [ + '"models".\'default\' = "ocx-retired"', + '\'ui\'."fork_secondary_model" = \'ocx-retired\'', + 'keep = "root"', + "", + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(['keep = "root"', "", ""].join("\n")); + }); + + test("adoption rewrites a quoted root dotted reference", () => { + const oldAlias = "ocx-gpt-5-6-sol-2"; + writeFileSync(configPath, [ + `"models".'default' = '${oldAlias}'`, + "", + `[model.${oldAlias}]`, + 'model = "gpt-5.6-sol"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + const defaultAlias = /^"models"\.'default' = "([^"]+)"$/m.exec(content)?.[1]; + expect(defaultAlias).toBeDefined(); + expect(defaultAlias).not.toBe(oldAlias); + expect(content).toContain(`[model.${defaultAlias}]`); + expect(content).not.toContain(`[model.${oldAlias}]`); + }); + + test("adoption rewrites an inline-table reference", () => { + const oldAlias = "ocx-gpt-5-6-sol-2"; + const decoys = Array.from({ length: 40 }, () => "default = 'not-a-key'").join(", "); + writeFileSync(configPath, [ + `models = { note = "{ ${decoys} }", default = "${oldAlias}", keep = true }`, + "", + `[model.${oldAlias}]`, + 'model = "gpt-5.6-sol"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + const defaultAlias = /models = \{ note = .*?, default = "([^"]+)", keep = true \}/.exec(content)?.[1]; + expect(defaultAlias).toBeDefined(); + expect(defaultAlias).not.toBe(oldAlias); + expect(content).toContain(`[model.${defaultAlias}]`); + }); + + test("teardown fails closed on an inline-table reference", () => { + const original = [ + 'models = { default = "ocx-retired", keep = true }', + "", + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n"); + writeFileSync(configPath, original); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: false, changed: false }); + expect(readFileSync(configPath, "utf8")).toBe(original); + }); + + test("semantic probing cannot confuse an existing sentinel-shaped alias", () => { + const unrelated = 'default = "keep"\n'; + const alias = `__opencodex_reference_probe_0_${unrelated.indexOf('"')}__`; + writeFileSync(configPath, unrelated + [ + `models.default = "${alias}"`, + "", + `[model.${alias}]`, + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe('default = "keep"\n\n'); + }); + + test("adoption prefers the managed survivor over a same-model user table", () => { + const oldAlias = "ocx-gpt-5-6-sol-2"; + writeFileSync(configPath, [ + "[models]", + `default = "${oldAlias}"`, + "", + "[model.manual]", + 'model = "gpt-5.6-sol"', + 'base_url = "https://example.com/v1"', + 'api_key = "user-secret"', + "", + `[model.${oldAlias}]`, + 'model = "gpt-5.6-sol"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + "", + ].join("\n")); + + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + const defaultAlias = /^default = "([^"]+)"$/m.exec(content)?.[1]; + expect(defaultAlias).toBeDefined(); + expect(defaultAlias).not.toBe("manual"); + expect(defaultAlias).not.toBe(oldAlias); + expect(content).toContain(`[model.${defaultAlias}]`); + }); + + test("teardown follows a non-contiguous ownership child table", () => { + const alias = "ocx-retired"; + const preserved = ["[other]", "keep = true", "", ""].join("\n"); + writeFileSync(configPath, [ + `[model.${alias}]`, + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + "[other]", + "keep = true", + "", + `[model.${alias}.extra_headers]`, + 'x-opencodex-grok = "1"', + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(preserved); + }); + + test("teardown follows an ownership child written before its parent", () => { + const alias = "ocx-retired"; + const preserved = ["[other]", "keep = true", "", ""].join("\n"); + writeFileSync(configPath, [ + `[model.${alias}.extra_headers]`, + 'x-opencodex-grok = "1"', + "", + "[other]", + "keep = true", + "", + `[model.${alias}]`, + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(preserved); + }); + + test("teardown follows an ownership child re-serialized beyond the fence", () => { + const alias = "ocx-retired"; + writeFileSync(configPath, [ + `[model.${alias}]`, + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join("\n")); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + writeFileSync(configPath, readFileSync(configPath, "utf8") + [ + `[model.${alias}.extra_headers]`, + 'x-opencodex-grok = "1"', + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8").trim()).toBe(""); + }); + + test("teardown follows a pre-fence ownership child to a post-fence parent", () => { + const alias = "ocx-retired"; + writeFileSync(configPath, [ + `[model.${alias}.extra_headers]`, + 'x-opencodex-grok = "1"', + "", + ].join("\n")); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + writeFileSync(configPath, readFileSync(configPath, "utf8") + [ + `[model.${alias}]`, + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + "", + ].join("\n")); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8").trim()).toBe(""); + }); + + test("a Unicode line separator inside a comment does not hide a user model header", () => { + const alias = "ocx-gpt-5-6-sol"; + writeFileSync(configPath, [ + `[model.${alias}] # alpha\u2028omega`, + 'model = "user/model"', + 'base_url = "https://example.com/v1"', + 'api_key = "user-secret"', + "", + ].join("\n")); + + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + expect(content).toContain(`[model.${alias}] # alpha\u2028omega`); + expect(content).toContain(`[model.${alias}-2]`); + }); + + test("teardown ignores generated-looking tables inside multiline TOML strings", () => { + for (const delimiter of ['"""', "'''"]) { + const original = [ + `notes = ${delimiter}`, + "[model.ocx-retired]", + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + delimiter, + "", + "[model.user-owned]", + 'model = "user/model"', + 'base_url = "https://example.com/v1"', + 'api_key = "user-secret"', + "", + ].join("\n"); + writeFileSync(configPath, original); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: false }); + expect(readFileSync(configPath, "utf8")).toBe(original); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: true }); + expect(readFileSync(configPath, "utf8")).toBe(original); + } + }); + + test("ownership keys inside a multiline value do not claim a manual table", () => { + for (const delimiter of ['"""', "'''"]) { + const original = [ + "[model.hand-written]", + `notes = ${delimiter}`, + 'model = "retired/model"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_key = "opencodex-loopback"', + OWNERSHIP_MARKER, + delimiter, + 'model = "user/model"', + 'base_url = "https://example.com/v1"', + 'api_key = "user-secret"', + "", + ].join("\n"); + writeFileSync(configPath, original); + + expect(stripGrokConfig({ grokHome })).toMatchObject({ ok: true, changed: false }); + expect(readFileSync(configPath, "utf8")).toBe(original); + } + }); + + test("adoption ignores fake survivors and references inside multiline strings", () => { + const oldAlias = "ocx-gpt-5-6-sol-2"; + writeFileSync(configPath, [ + "[models]", + `default = "${oldAlias}"`, + 'notes = """', + "[model.fake-survivor]", + 'model = "gpt-5.6-sol"', + `default = "${oldAlias}"`, + '"""', + "", + `[model.${oldAlias}]`, + 'model = "gpt-5.6-sol"', + 'base_url = "http://127.0.0.1:10100/v1"', + 'api_backend = "chat_completions"', + 'api_key = "opencodex-loopback"', + 'name = "OCX gpt-5.6-sol"', + "", + ].join("\n")); + + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + const content = readFileSync(configPath, "utf8"); + expect(content).toContain('default = "ocx-gpt-5-6-sol"'); + expect(content).toContain(`[model.fake-survivor]\nmodel = "gpt-5.6-sol"\ndefault = "${oldAlias}"`); + expect(content).not.toContain(`[model.${oldAlias}]`); + }); + test("markerless teardown preserves an ambiguous legacy row", () => { const legacy = [ "[model.ocx-gpt-5-6-sol]", From 0ed3f351c5c9a05f984b264759498e8fbffa8792 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 02:55:00 +0900 Subject: [PATCH 5/8] test(grok): assert retired orphan injection (cherry picked from commit 019c792607808614a0b9f61be1c9aa9170733d7e) --- tests/grok-orphan-adoption.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index 2b36fd0956..300d5c9649 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -212,8 +212,10 @@ describe("Grok orphan adoption (#511)", () => { "", ].join("\n")); - injectGrokConfig(10100, MODELS, { grokHome }); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); const content = readFileSync(configPath, "utf8"); + expect(content).toContain(BEGIN_MARKER); expect(content).toContain('default = "ocx-retired"'); expect(content).toContain('fork_secondary_model = "ocx-retired"'); expect(content).toContain("[model.ocx-retired]"); From ecd6225fcd88e355dfb735ea8d67f3a1d8353ca5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 05:06:53 +0900 Subject: [PATCH 6/8] fix(grok): clear references to removed models When orphan adoption removes an excluded managed model without emitting a replacement table, remove its semantic model references through the existing TOML-safe transform instead of leaving dangling aliases. --- src/grok/inject.ts | 20 ++++++++++---------- tests/grok-orphan-adoption.test.ts | 20 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/grok/inject.ts b/src/grok/inject.ts index 7d1478811f..4cb416efaf 100644 --- a/src/grok/inject.ts +++ b/src/grok/inject.ts @@ -713,9 +713,9 @@ function transformAliasReferences( return next; } -/** Repoint references at whichever alias survived orphan adoption. */ -function rewriteAliasReferences(content: string, renames: Map): string { - return transformAliasReferences(content, renames); +/** Repoint references at whichever alias survived orphan adoption, or remove them. */ +function rewriteAliasReferences(content: string, replacements: Map): string { + return transformAliasReferences(content, replacements); } /** Remove only references that name model aliases teardown actually swept. */ @@ -916,9 +916,9 @@ export function injectGrokConfig( nextContent = `${content}\n${block}\n`; } - // Repoint `default` / `fork_secondary_model` at whichever alias survived. A removed - // model with no replacement keeps its reference untouched — a stale name in a working - // file beats a dangling one. + // Repoint `default` / `fork_secondary_model` at whichever alias survived. If an + // excluded or removed model has no replacement, clear its references with the same + // TOML-aware transform used by teardown so the new config cannot point at a deleted table. if (orphans.length > 0) { const survivors = new Map(); const structure = analyzeTomlStructure(nextContent); @@ -931,12 +931,12 @@ export function injectGrokConfig( const modelId = tableBodyKeys(nextContent.slice(header.index + header.length, bodyEnd)).get("model"); if (modelId !== undefined && !survivors.has(modelId)) survivors.set(modelId, alias); } - const renames = new Map(); + const replacements = new Map(); for (const orphan of orphans) { - const replacement = survivors.get(orphan.modelId); - if (replacement && replacement !== orphan.alias) renames.set(orphan.alias, replacement); + const replacement = survivors.get(orphan.modelId) ?? null; + if (replacement !== orphan.alias) replacements.set(orphan.alias, replacement); } - nextContent = rewriteAliasReferences(nextContent, renames); + nextContent = rewriteAliasReferences(nextContent, replacements); } const output = applyEol(nextContent, eol); diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index 300d5c9649..381ab94144 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -878,6 +878,26 @@ describe("Grok orphan adoption (#511)", () => { expect(modelTables(readFileSync(configPath, "utf8"))).toEqual([]); }); + test("clears references when an excluded model has no survivor (#2830)", () => { + writeOrphanedConfig([ + OWNERSHIP_MARKER, + "", + "[ui]", + 'fork_secondary_model = "ocx-gpt-5-6-sol"', + "", + ].join("\n")); + + expect(injectGrokConfig(10100, MODELS, { + grokHome, + excluded: new Set(["gpt-5.6-sol"]), + })).toMatchObject({ ok: true, changed: true }); + + const content = readFileSync(configPath, "utf8"); + expect(modelTables(content)).toEqual([]); + expect(content).not.toContain('default = "ocx-gpt-5-6-sol"'); + expect(content).not.toContain('fork_secondary_model = "ocx-gpt-5-6-sol"'); + }); + // F7: the sweep must converge, or `changed` is meaningless to callers. test("is idempotent: the second sync reports no change", () => { writeOrphanedConfig(); From 9abfabf180f7f8cd6591584ae60cba75402564f5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 05:51:35 +0900 Subject: [PATCH 7/8] fix(grok): clean every removed model reference Derive removed aliases from the previous managed block as well as adopted orphans, then rewrite every Grok model-selector path through one declared inventory. Cover the normal managed exclusion path across all current fields. --- src/grok/inject.ts | 400 ++++++++++++++++++----------- tests/grok-orphan-adoption.test.ts | 54 +++- 2 files changed, 294 insertions(+), 160 deletions(-) diff --git a/src/grok/inject.ts b/src/grok/inject.ts index 4cb416efaf..f45c38fdde 100644 --- a/src/grok/inject.ts +++ b/src/grok/inject.ts @@ -168,73 +168,6 @@ function tomlContainerEnd(content: string, start: number): number | null { return null; } -/** Locate one top-level string member inside an inline table without scanning nested prose. */ -function tomlInlineStringValueSpan( - content: string, - start: number, - end: number, - targetKey: string, -): { start: number; end: number } | null { - const skipTrivia = (from: number): number => { - let cursor = from; - while (cursor < end) { - while (/[ \t\r\n]/.test(content[cursor] ?? "")) cursor += 1; - if (content[cursor] !== "#") break; - const newline = content.indexOf("\n", cursor); - cursor = newline === -1 || newline >= end ? end : newline + 1; - } - return cursor; - }; - const keyPattern = new RegExp(DOTTED_KEY, "y"); - let entryStart = start + 1; - while (entryStart < end - 1) { - entryStart = skipTrivia(entryStart); - if (entryStart >= end - 1 || content[entryStart] === "}") return null; - keyPattern.lastIndex = entryStart; - const key = keyPattern.exec(content); - if (key === null) return null; - let cursor = skipTrivia(keyPattern.lastIndex); - if (content[cursor] !== "=") return null; - const valueStart = skipTrivia(cursor + 1); - const segments = canonicalDottedKey(key[0]); - if (segments.length === 1 && segments[0] === targetKey) { - const value = tomlStringSpanAt(content, valueStart); - return value === null ? null : { start: valueStart, end: value.end }; - } - - const stack: string[] = []; - cursor = valueStart; - let foundNext = false; - while (cursor < end - 1) { - const char = content[cursor]!; - if (char === "#") { - const newline = content.indexOf("\n", cursor); - cursor = newline === -1 || newline >= end ? end : newline + 1; - continue; - } - if (char === '"' || char === "'") { - const value = tomlStringSpanAt(content, cursor); - if (value === null) return null; - cursor = value.end; - continue; - } - if (char === "{" || char === "[") stack.push(char); - else if (char === "}" || char === "]") { - if (stack.length === 0) return null; - const expected = char === "}" ? "{" : "["; - if (stack.pop() !== expected) return null; - } else if (char === "," && stack.length === 0) { - entryStart = cursor + 1; - foundNext = true; - break; - } - cursor += 1; - } - if (!foundNext) return null; - } - return null; -} - /** * A same-length lexical projection for structural scans. Triple-quoted string bytes become * spaces while line endings and every byte outside those values keep their original offsets. @@ -568,18 +501,40 @@ function removeOrphanTables(content: string, orphans: OrphanTable[]): string { return removeTableRanges(content, orphanRanges(orphans)); } +/** Model aliases and routed ids owned by one complete managed region. */ +function managedModelAliases(content: string, region: ManagedRegion | null): Map { + const models = new Map(); + if (!region) return models; + const structure = analyzeTomlStructure(content); + for (const [position, header] of structure.headers.entries()) { + if (header.array || header.segments.length !== 2 || header.segments[0] !== "model") continue; + if (header.index < region.start || header.index >= region.end) continue; + const bodyEnd = Math.min(structure.headers[position + 1]?.index ?? content.length, region.end); + const modelId = tableBodyKeys(content.slice(header.index + header.length, bodyEnd)).get("model"); + if (modelId !== undefined) models.set(header.segments[1]!, modelId); + } + return models; +} + /** Read one exact path from an already parsed TOML document. */ -function tomlPathString(document: unknown, path: readonly string[]): string | null { +type TomlPathSegment = string | number; + +function tomlPathString(document: unknown, path: readonly TomlPathSegment[]): string | null { let value = document; for (const segment of path) { - if (typeof value !== "object" || value === null || Array.isArray(value)) return null; - value = (value as Record)[segment]; + if (typeof segment === "number") { + if (!Array.isArray(value)) return null; + value = value[segment]; + } else { + if (typeof value !== "object" || value === null || Array.isArray(value)) return null; + value = (value as Record)[segment]; + } } return typeof value === "string" ? value : null; } /** Parse a probe document and read one exact semantic path. */ -function parsedTomlPathString(content: string, path: readonly string[]): string | null { +function parsedTomlPathString(content: string, path: readonly TomlPathSegment[]): string | null { try { return tomlPathString(Bun.TOML.parse(content), path); } catch { @@ -587,7 +542,139 @@ function parsedTomlPathString(content: string, path: readonly string[]): string } } -/** Rename or remove the two semantic model references without touching user prose. */ +type ModelReferencePatternSegment = string | "*"; + +interface ModelReferencePath { + path: readonly ModelReferencePatternSegment[]; + /** A structured reference assignment that can be removed whole without losing sibling config. */ + removableContainerPath?: readonly string[]; +} + +/** Grok config values whose strings resolve through the `[model.]` catalog. */ +const MODEL_REFERENCE_PATHS: readonly ModelReferencePath[] = [ + { path: ["models", "default"] }, + { path: ["models", "web_search"] }, + { path: ["models", "session_summary"] }, + { path: ["models", "image_description"] }, + { path: ["models", "prompt_suggestion"] }, + { path: ["ui", "fork_secondary_model"] }, + { path: ["subagents", "models", "*"] }, + { path: ["auto_mode", "classifier_model"] }, + { + path: ["goal", "planner_model", "model"], + removableContainerPath: ["goal", "planner_model"], + }, + { + path: ["goal", "strategist_model", "model"], + removableContainerPath: ["goal", "strategist_model"], + }, + { + path: ["goal", "skeptic_models", "*", "model"], + removableContainerPath: ["goal", "skeptic_models"], + }, +]; + +interface AliasReference { + path: TomlPathSegment[]; + alias: string; + removableContainerPath?: readonly string[]; +} + +function collectAliasReferences(document: unknown): AliasReference[] { + const references: AliasReference[] = []; + const visit = ( + value: unknown, + pattern: readonly ModelReferencePatternSegment[], + patternIndex: number, + path: TomlPathSegment[], + removableContainerPath: readonly string[] | undefined, + ): void => { + if (patternIndex === pattern.length) { + if (typeof value === "string") { + references.push({ + path, + alias: value, + ...(removableContainerPath ? { removableContainerPath } : {}), + }); + } + return; + } + const segment = pattern[patternIndex]!; + if (segment === "*") { + if (Array.isArray(value)) { + for (const [index, item] of value.entries()) { + visit(item, pattern, patternIndex + 1, [...path, index], removableContainerPath); + } + } else if (typeof value === "object" && value !== null) { + for (const [key, item] of Object.entries(value)) { + visit(item, pattern, patternIndex + 1, [...path, key], removableContainerPath); + } + } + return; + } + if (typeof value !== "object" || value === null || Array.isArray(value)) return; + visit( + (value as Record)[segment], + pattern, + patternIndex + 1, + [...path, segment], + removableContainerPath, + ); + }; + + for (const reference of MODEL_REFERENCE_PATHS) { + visit(document, reference.path, 0, [], reference.removableContainerPath); + } + return references; +} + +function sourcePath(path: readonly TomlPathSegment[]): string[] { + return path.filter((segment): segment is string => typeof segment === "string"); +} + +function pathsEqual(left: readonly string[], right: readonly string[]): boolean { + return left.length === right.length && left.every((segment, index) => segment === right[index]); +} + +function pathStartsWith(path: readonly string[], prefix: readonly string[]): boolean { + return path.length >= prefix.length + && prefix.every((segment, index) => segment === path[index]); +} + +function tomlContainerStringSpans( + content: string, + start: number, + end: number, +): Array<{ start: number; end: number }> { + const spans: Array<{ start: number; end: number }> = []; + for (let index = start + 1; index < end - 1;) { + const char = content[index]!; + if (char === "#") { + const newline = content.indexOf("\n", index); + index = newline === -1 || newline >= end ? end : newline + 1; + continue; + } + if (char === '"' || char === "'") { + const span = tomlStringSpanAt(content, index); + if (span === null || span.end > end) return []; + spans.push({ start: index, end: span.end }); + index = span.end; + continue; + } + index += 1; + } + return spans; +} + +interface AliasReferenceCandidate { + valueStart: number; + valueEnd: number; + assignmentPath: string[]; + directLine: { start: number; end: number } | null; + containerLine: { start: number; end: number } | null; +} + +/** Rename or remove every declared semantic model reference without touching user prose. */ function transformAliasReferences( content: string, replacements: ReadonlyMap, @@ -602,14 +689,12 @@ function transformAliasReferences( "Grok config rewrite refused: Bun could not parse the TOML document safely.", ); } + const references = collectAliasReferences(document); + const targets = references.filter(reference => replacements.has(reference.alias)); + if (targets.length === 0) return content; const structure = analyzeTomlStructure(content); const edits: Array<{ start: number; end: number; replacement: string }> = []; - const candidates: Array<{ - pathKey: "models.default" | "ui.fork_secondary_model"; - valueStart: number; - valueEnd: number; - line: { start: number; end: number } | null; - }> = []; + const candidates: AliasReferenceCandidate[] = []; const assignment = new RegExp(String.raw`^([ \t]*(${DOTTED_KEY})[ \t]*=)`, "gm"); let headerPosition = -1; for (const match of structure.view.matchAll(assignment)) { @@ -618,64 +703,56 @@ function transformAliasReferences( while ((structure.headers[headerPosition + 1]?.index ?? Number.POSITIVE_INFINITY) < assignmentStart) headerPosition += 1; const currentHeader = headerPosition >= 0 ? structure.headers[headerPosition]! : null; - if (currentHeader?.array) continue; if (!allowRootDotted && currentHeader === null) continue; const segments = canonicalDottedKey(match[2]!); - const semanticPath = [...(currentHeader?.segments ?? []), ...segments]; + const assignmentPath = [...(currentHeader?.segments ?? []), ...segments]; let valueStart = assignmentStart + match[1]!.length; while (content[valueStart] === " " || content[valueStart] === "\t") valueStart += 1; - const pathKey = semanticPath.length === 2 && semanticPath[0] === "models" - && semanticPath[1] === "default" - ? "models.default" - : semanticPath.length === 2 && semanticPath[0] === "ui" - && semanticPath[1] === "fork_secondary_model" - ? "ui.fork_secondary_model" - : null; - if (pathKey !== null) { + const directTargets = targets.filter(target => pathsEqual(sourcePath(target.path), assignmentPath)); + if (directTargets.length > 0) { const value = tomlStringSpanAt(content, valueStart); - if (value === null) continue; - const suffix = /^[ \t]*(?:#[^\r\n]*)?(?:\r?\n|$)/.exec(content.slice(value.end)); - if (suffix === null) continue; + if (value !== null) { + const suffix = /^[ \t]*(?:#[^\r\n]*)?(?:\r?\n|$)/.exec(content.slice(value.end)); + if (suffix !== null) { + candidates.push({ + valueStart, + valueEnd: value.end, + assignmentPath, + directLine: { start: assignmentStart, end: value.end + suffix[0].length }, + containerLine: null, + }); + continue; + } + } + } + + const containerTargets = targets.filter(target => + pathStartsWith(sourcePath(target.path), assignmentPath)); + if (containerTargets.length === 0 || (content[valueStart] !== "{" && content[valueStart] !== "[")) continue; + const containerEnd = tomlContainerEnd(content, valueStart); + if (containerEnd === null) continue; + const suffix = /^[ \t]*(?:#[^\r\n]*)?(?:\r?\n|$)/.exec(content.slice(containerEnd)); + if (suffix === null) continue; + const containerLine = { start: assignmentStart, end: containerEnd + suffix[0].length }; + for (const value of tomlContainerStringSpans(content, valueStart, containerEnd)) { candidates.push({ - pathKey, - valueStart, + valueStart: value.start, valueEnd: value.end, - line: { start: assignmentStart, end: value.end + suffix[0].length }, + assignmentPath, + directLine: null, + containerLine, }); - continue; } - - // The only non-line form we support is a root inline table (`models = { ... }` / `ui =`). - const inlineTarget = currentHeader === null && semanticPath.length === 1 - && semanticPath[0] === "models" - ? { pathKey: "models.default" as const, key: "default" } - : currentHeader === null && semanticPath.length === 1 && semanticPath[0] === "ui" - ? { pathKey: "ui.fork_secondary_model" as const, key: "fork_secondary_model" } - : null; - if (inlineTarget === null || content[valueStart] !== "{") continue; - const inlineEnd = tomlContainerEnd(content, valueStart); - if (inlineEnd === null) continue; - const value = tomlInlineStringValueSpan(content, valueStart, inlineEnd, inlineTarget.key); - if (value === null) continue; - candidates.push({ - pathKey: inlineTarget.pathKey, - valueStart: value.start, - valueEnd: value.end, - line: null, - }); } - const targets = [ - { path: ["models", "default"] as const, pathKey: "models.default" as const }, - { path: ["ui", "fork_secondary_model"] as const, pathKey: "ui.fork_secondary_model" as const }, - ]; for (const [targetIndex, target] of targets.entries()) { - const currentAlias = tomlPathString(document, target.path); - if (currentAlias === null || !replacements.has(currentAlias)) continue; - const replacement = replacements.get(currentAlias)!; - const probeCandidates = candidates.filter(candidate => candidate.pathKey === target.pathKey); + const replacement = replacements.get(target.alias)!; + const targetSourcePath = sourcePath(target.path); + const probeCandidates = candidates.filter(candidate => + pathsEqual(candidate.assignmentPath, targetSourcePath) + || pathStartsWith(targetSourcePath, candidate.assignmentPath)); if (probeCandidates.length === 0 && !allowRootDotted) continue; - if (probeCandidates.length === 0 || probeCandidates.length > 32) { + if (probeCandidates.length === 0 || probeCandidates.length > 128) { throw new Error( "Grok config rewrite refused: the model-reference source could not be bounded safely.", ); @@ -683,20 +760,37 @@ function transformAliasReferences( let located = false; for (const candidate of probeCandidates) { let sentinel = `__opencodex_reference_probe_${targetIndex}_${candidate.valueStart}__`; - while (sentinel === currentAlias) sentinel += "_"; + while (sentinel === target.alias) sentinel += "_"; const probe = content.slice(0, candidate.valueStart) + tomlString(sentinel) + content.slice(candidate.valueEnd); if (parsedTomlPathString(probe, target.path) !== sentinel) continue; - if (replacement === null && candidate.line === null) { - throw new Error( - "Grok teardown refused: a model reference uses an inline TOML shape that cannot " - + "be removed without rewriting user-owned bytes.", - ); + if (replacement === null) { + let removal = candidate.directLine; + if (removal === null && candidate.containerLine !== null + && target.removableContainerPath + && pathsEqual(candidate.assignmentPath, target.removableContainerPath)) { + const containerReferences = references.filter(reference => + pathStartsWith(sourcePath(reference.path), candidate.assignmentPath)); + if (containerReferences.length > 0 + && containerReferences.every(reference => replacements.get(reference.alias) === null)) { + removal = candidate.containerLine; + } + } + if (removal === null) { + throw new Error( + "Grok teardown refused: a model reference uses an inline TOML shape that cannot " + + "be removed without rewriting user-owned bytes.", + ); + } + edits.push({ start: removal.start, end: removal.end, replacement: "" }); + } else { + edits.push({ + start: candidate.valueStart, + end: candidate.valueEnd, + replacement: tomlString(replacement), + }); } - edits.push(replacement === null - ? { start: candidate.line!.start, end: candidate.line!.end, replacement: "" } - : { start: candidate.valueStart, end: candidate.valueEnd, replacement: tomlString(replacement) }); located = true; break; } @@ -707,7 +801,8 @@ function transformAliasReferences( } } let next = content; - for (const edit of edits.sort((a, b) => b.start - a.start)) { + const uniqueEdits = new Map(edits.map(edit => [`${edit.start}:${edit.end}:${edit.replacement}`, edit])); + for (const edit of [...uniqueEdits.values()].sort((a, b) => b.start - a.start)) { next = next.slice(0, edit.start) + edit.replacement + next.slice(edit.end); } return next; @@ -874,6 +969,7 @@ export function injectGrokConfig( // Ambiguous fence: refuse before the sweep, or "outside the region" could mean the // entire file. if (originalRegion?.orphaned) return orphanedMarkerResult("injection"); + const previousManagedModels = managedModelAliases(originalContent, originalRegion); // Adopt our own pre-fence entries (#511) BEFORE reserving user aliases, so the stale // duplicate is replaced instead of routed around forever. Runs inside the normalized @@ -916,28 +1012,24 @@ export function injectGrokConfig( nextContent = `${content}\n${block}\n`; } - // Repoint `default` / `fork_secondary_model` at whichever alias survived. If an - // excluded or removed model has no replacement, clear its references with the same - // TOML-aware transform used by teardown so the new config cannot point at a deleted table. - if (orphans.length > 0) { - const survivors = new Map(); - const structure = analyzeTomlStructure(nextContent); - const managedRegion = findManagedRegion(nextContent); - for (const [position, header] of structure.headers.entries()) { - if (header.array || header.segments.length !== 2 || header.segments[0] !== "model") continue; - if (!managedRegion || header.index < managedRegion.start || header.index >= managedRegion.end) continue; - const alias = header.segments[1]!; - const bodyEnd = structure.headers[position + 1]?.index ?? nextContent.length; - const modelId = tableBodyKeys(nextContent.slice(header.index + header.length, bodyEnd)).get("model"); - if (modelId !== undefined && !survivors.has(modelId)) survivors.set(modelId, alias); - } - const replacements = new Map(); - for (const orphan of orphans) { - const replacement = survivors.get(orphan.modelId) ?? null; - if (replacement !== orphan.alias) replacements.set(orphan.alias, replacement); - } - nextContent = rewriteAliasReferences(nextContent, replacements); + // Repoint every model selector at whichever managed alias survived. Compare both swept + // out-of-fence tables and the PREVIOUS managed block: ordinary exclusion removes only the + // latter, so tying cleanup to `orphans` made the #2830 path dead code. + const nextManagedModels = managedModelAliases(nextContent, findManagedRegion(nextContent)); + const survivors = new Map(); + for (const [alias, modelId] of nextManagedModels) { + if (!survivors.has(modelId)) survivors.set(modelId, alias); + } + const replacements = new Map(); + for (const removed of [ + ...orphans.map(orphan => ({ alias: orphan.alias, modelId: orphan.modelId })), + ...[...previousManagedModels].map(([alias, modelId]) => ({ alias, modelId })), + ]) { + if (nextManagedModels.get(removed.alias) === removed.modelId) continue; + const replacement = survivors.get(removed.modelId) ?? null; + if (replacement !== removed.alias) replacements.set(removed.alias, replacement); } + nextContent = rewriteAliasReferences(nextContent, replacements); const output = applyEol(nextContent, eol); if (output === rawContent) { diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index 381ab94144..fee9bba318 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -58,6 +58,20 @@ describe("Grok orphan adoption (#511)", () => { return [...content.matchAll(/^\[model\.([^\]]+)\]$/gm)].map(match => match[1]!); } + function countStringValue(value: unknown, target: string): number { + if (value === target) return 1; + if (Array.isArray(value)) { + return value.reduce((count, item) => count + countStringValue(item, target), 0); + } + if (typeof value === "object" && value !== null) { + return Object.values(value).reduce( + (count, item) => count + countStringValue(item, target), + 0, + ); + } + return 0; + } + test("adopts the stale entry so exactly one table per model survives", () => { writeOrphanedConfig(); const result = injectGrokConfig(10100, MODELS, { grokHome }); @@ -878,15 +892,44 @@ describe("Grok orphan adoption (#511)", () => { expect(modelTables(readFileSync(configPath, "utf8"))).toEqual([]); }); - test("clears references when an excluded model has no survivor (#2830)", () => { - writeOrphanedConfig([ - OWNERSHIP_MARKER, + test("managed exclusion leaves zero references to a removed model (#2830)", () => { + const alias = "ocx-gpt-5-6-sol"; + writeFileSync(configPath, [ + "[models]", + `default = "${alias}"`, + `web_search = "${alias}"`, + `session_summary = "${alias}"`, + `image_description = "${alias}"`, + `prompt_suggestion = "${alias}"`, "", "[ui]", - 'fork_secondary_model = "ocx-gpt-5-6-sol"', + `fork_secondary_model = "${alias}"`, + "", + "[subagents.models]", + `explore = "${alias}"`, + "", + "[auto_mode]", + `classifier_model = "${alias}"`, + "", + "[goal]", + `planner_model = { model = "${alias}", agent_type = "grok-build-plan" }`, + "", + "[goal.strategist_model]", + `model = "${alias}"`, + 'agent_type = "cursor"', + "", + "[[goal.skeptic_models]]", + `model = "${alias}"`, + 'agent_type = "grok-build-plan"', "", ].join("\n")); + expect(injectGrokConfig(10100, MODELS, { grokHome })) + .toMatchObject({ ok: true, changed: true }); + const activeContent = readFileSync(configPath, "utf8"); + expect(modelTables(activeContent)).toEqual([alias]); + expect(countStringValue(Bun.TOML.parse(activeContent), alias)).toBe(11); + expect(injectGrokConfig(10100, MODELS, { grokHome, excluded: new Set(["gpt-5.6-sol"]), @@ -894,8 +937,7 @@ describe("Grok orphan adoption (#511)", () => { const content = readFileSync(configPath, "utf8"); expect(modelTables(content)).toEqual([]); - expect(content).not.toContain('default = "ocx-gpt-5-6-sol"'); - expect(content).not.toContain('fork_secondary_model = "ocx-gpt-5-6-sol"'); + expect(countStringValue(Bun.TOML.parse(content), alias)).toBe(0); }); // F7: the sweep must converge, or `changed` is meaningless to callers. From 1a73b7a11312782f05824666515683271f735ca8 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 06:06:39 +0900 Subject: [PATCH 8/8] fix(grok): clean inline subagent model references Extend managed-model cleanup to role and persona model selectors, and cover the complete inline selector matrix with a zero-dangling-reference regression. --- src/grok/inject.ts | 2 ++ tests/grok-orphan-adoption.test.ts | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/grok/inject.ts b/src/grok/inject.ts index f45c38fdde..5412dcfe5a 100644 --- a/src/grok/inject.ts +++ b/src/grok/inject.ts @@ -559,6 +559,8 @@ const MODEL_REFERENCE_PATHS: readonly ModelReferencePath[] = [ { path: ["models", "prompt_suggestion"] }, { path: ["ui", "fork_secondary_model"] }, { path: ["subagents", "models", "*"] }, + { path: ["subagents", "roles", "*", "model"] }, + { path: ["subagents", "personas", "*", "model"] }, { path: ["auto_mode", "classifier_model"] }, { path: ["goal", "planner_model", "model"], diff --git a/tests/grok-orphan-adoption.test.ts b/tests/grok-orphan-adoption.test.ts index fee9bba318..a35c532012 100644 --- a/tests/grok-orphan-adoption.test.ts +++ b/tests/grok-orphan-adoption.test.ts @@ -908,6 +908,14 @@ describe("Grok orphan adoption (#511)", () => { "[subagents.models]", `explore = "${alias}"`, "", + "[subagents.roles.reviewer]", + `model = "${alias}"`, + 'description = "Review code"', + "", + "[subagents.personas.concise]", + `model = "${alias}"`, + 'instructions = "Be concise"', + "", "[auto_mode]", `classifier_model = "${alias}"`, "", @@ -928,7 +936,7 @@ describe("Grok orphan adoption (#511)", () => { .toMatchObject({ ok: true, changed: true }); const activeContent = readFileSync(configPath, "utf8"); expect(modelTables(activeContent)).toEqual([alias]); - expect(countStringValue(Bun.TOML.parse(activeContent), alias)).toBe(11); + expect(countStringValue(Bun.TOML.parse(activeContent), alias)).toBe(13); expect(injectGrokConfig(10100, MODELS, { grokHome, @@ -938,6 +946,8 @@ describe("Grok orphan adoption (#511)", () => { const content = readFileSync(configPath, "utf8"); expect(modelTables(content)).toEqual([]); expect(countStringValue(Bun.TOML.parse(content), alias)).toBe(0); + expect(content).toContain('[subagents.roles.reviewer]\ndescription = "Review code"'); + expect(content).toContain('[subagents.personas.concise]\ninstructions = "Be concise"'); }); // F7: the sweep must converge, or `changed` is meaningless to callers.