From 5916df185df27d634b4906bfceba880eea20c3e5 Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 01:42:13 +0900 Subject: [PATCH 1/6] fix(server): stop a chmod from fencing the data plane behind 503 The package-tree guard compared the inode CHANGE time, which moves for metadata writes that replace nothing. A chmod, a chown, a touch, an editor normalizing permissions, a backup tool restoring modes - each left device, inode and size identical, so the guard's own definition of "replaced" was met by a file nobody had replaced. Every /v1/* request then answered 503 with "restart OpenCodex before retrying", and since a negative reading is deliberately never cached, there was no recovery short of a restart. Comparing the CONTENT modification time instead. Measured on macOS before changing anything: a chmod moves ctime and leaves mtime alone, an in-place rewrite of the same byte length moves mtime while inode and size hold, and an atomic write-then-rename install changes the inode as well. So mtime drops the false positive and keeps every real detection. The three new tests drive the real filesystem rather than a hand-built observation, because the defect was in which stat field was read and a synthetic fixture cannot tell the two apart - it would have passed before and after. Confirmed by mutation: putting ctimeNs back turns the permission test red and leaves the two replacement tests green. The guard's comment claimed it was detecting "an event that happens at most once per install". That is true of a tree replacement and was never true of a ctime change. --- src/lib/package-tree-integrity.ts | 17 ++++++-- tests/package-tree-integrity.test.ts | 65 +++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/src/lib/package-tree-integrity.ts b/src/lib/package-tree-integrity.ts index 55554dd208..1d6d910de5 100644 --- a/src/lib/package-tree-integrity.ts +++ b/src/lib/package-tree-integrity.ts @@ -3,7 +3,7 @@ import { statSync } from "node:fs"; export interface PackageTreeObservation { readonly device: bigint; readonly inode: bigint; - readonly changeTimeNs: bigint; + readonly contentTimeNs: bigint; readonly size: bigint; } @@ -25,7 +25,18 @@ function observePackageManifest(): PackageTreeObservation | null { return { device: stat.dev, inode: stat.ino, - changeTimeNs: stat.ctimeNs, + // mtimeNs, NOT ctimeNs. An inode-change time moves for METADATA writes that + // replace nothing: chmod, chown, touch, an editor normalizing permissions, a + // backup tool restoring modes. Each of those left device, inode and size + // identical, so the comparison below called the manifest "replaced" and every + // /v1/* request answered 503 until the process was restarted. Measured on + // macOS: chmod alone moved ctimeNs and left mtimeNs untouched. + // + // mtimeNs still catches every real replacement. An in-place rewrite of the + // same byte length moves mtimeNs while inode and size hold; an atomic + // install (write-then-rename, which is what a package manager does) changes + // the inode as well. Both were measured before this change was made. + contentTimeNs: stat.mtimeNs, size: stat.size, }; } catch { @@ -36,7 +47,7 @@ function observePackageManifest(): PackageTreeObservation | null { function sameObservation(left: PackageTreeObservation, right: PackageTreeObservation): boolean { return left.device === right.device && left.inode === right.inode - && left.changeTimeNs === right.changeTimeNs + && left.contentTimeNs === right.contentTimeNs && left.size === right.size; } diff --git a/tests/package-tree-integrity.test.ts b/tests/package-tree-integrity.test.ts index 47902c65b1..d8bb637990 100644 --- a/tests/package-tree-integrity.test.ts +++ b/tests/package-tree-integrity.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, rmSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, renameSync, rmSync, statSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { saveConfig } from "../src/config"; import { @@ -49,7 +49,7 @@ describe("package tree integrity", () => { let observation: PackageTreeObservation = { device: 1n, inode: 10n, - changeTimeNs: 100n, + contentTimeNs: 100n, size: 500n, }; // An explicit clock: `status()` reuses an `ok` reading for a second so the guard does not @@ -60,14 +60,14 @@ describe("package tree integrity", () => { expect(guard.status()).toEqual({ ok: true }); - observation = { ...observation, inode: 11n, changeTimeNs: 200n }; + observation = { ...observation, inode: 11n, contentTimeNs: 200n }; clock += 2_000; expect(guard.status()).toEqual({ ok: false, reason: "package_tree_replaced" }); }); test("an ok reading is reused briefly, and a bad one is never cached", () => { let observation: PackageTreeObservation | null = { - device: 1n, inode: 10n, changeTimeNs: 100n, size: 500n, + device: 1n, inode: 10n, contentTimeNs: 100n, size: 500n, }; let observations = 0; let clock = 0; @@ -96,7 +96,7 @@ describe("package tree integrity", () => { let observation: PackageTreeObservation | null = { device: 1n, inode: 10n, - changeTimeNs: 100n, + contentTimeNs: 100n, size: 500n, }; const guard = createPackageTreeIntegrityGuard(() => observation); @@ -105,6 +105,61 @@ describe("package tree integrity", () => { expect(guard.status()).toEqual({ ok: false, reason: "package_tree_unreadable" }); }); + // BUG-R1: a chmod fenced the whole data plane behind 503. + // + // These three drive the REAL filesystem rather than a hand-built observation, + // because the defect lived in which stat field was read - a synthetic + // PackageTreeObservation cannot tell ctime from mtime, so a fixture-only test + // would have passed both before and after the fix. + const manifest = () => join(TEST_DIR, "package.json"); + const observeAt = (path: string) => () => { + const stat = statSync(path, { bigint: true }); + return { + device: stat.dev, + inode: stat.ino, + contentTimeNs: stat.mtimeNs, + size: stat.size, + }; + }; + + test("a permission change is not a replacement", () => { + writeFileSync(manifest(), '{"name":"ocx","version":"1.0.0"}'); + let clock = 0; + const guard = createPackageTreeIntegrityGuard(observeAt(manifest()), () => clock); + expect(guard.status()).toEqual({ ok: true }); + + chmodSync(manifest(), 0o600); + clock += 2_000; + expect(guard.status()).toEqual({ ok: true }); + }); + + test("an in-place rewrite of the same byte length is still a replacement", () => { + writeFileSync(manifest(), '{"name":"ocx","version":"1.0.0"}'); + let clock = 0; + const guard = createPackageTreeIntegrityGuard(observeAt(manifest()), () => clock); + expect(guard.status()).toEqual({ ok: true }); + + // Same length, different bytes: neither inode nor size moves, so mtime is the + // only signal left. This is the case that would break if someone "simplified" + // the comparison down to inode and size. + writeFileSync(manifest(), '{"name":"ocx","version":"9.9.9"}'); + clock += 2_000; + expect(guard.status()).toEqual({ ok: false, reason: "package_tree_replaced" }); + }); + + test("an atomic install is still a replacement", () => { + writeFileSync(manifest(), '{"name":"ocx","version":"1.0.0"}'); + let clock = 0; + const guard = createPackageTreeIntegrityGuard(observeAt(manifest()), () => clock); + expect(guard.status()).toEqual({ ok: true }); + + // write-then-rename, which is what a package manager actually does. + writeFileSync(join(TEST_DIR, "package.json.new"), '{"name":"ocx","version":"1.0.0"}'); + renameSync(join(TEST_DIR, "package.json.new"), manifest()); + clock += 2_000; + expect(guard.status()).toEqual({ ok: false, reason: "package_tree_replaced" }); + }); + test("degrades health and refuses Responses requests with a restart-required error", async () => { saveConfig(config()); const packageTreeIntegrity = { From d66252ec0e367d4c2d083fefa206ee60aef5b4dc Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 01:46:23 +0900 Subject: [PATCH 2/6] fix(codex): require a dashboard session to rewrite the prompt stack /api/codex-prompt writes the user's config.toml - the file that decides what the model reads - and its write verbs accepted the raw admin token. The auth gate checks that token before it ever consults the session table, so any process that can read ~/.opencodex/admin-api-token could rewrite a prompt. AGENTS.md is explicit that this is the case the session requirement exists to stop. Mutating verbs now require the gui-session principal, the same check the star endpoint uses and for the same reason. Reads stay open to the admin token: describing the layer stack changes nothing and the CLI parity path depends on it. The honest limit, which the code comment states rather than implies: a process running as the user can mint its own session from the loopback dashboard bootstrap, and can edit config.toml directly without going through this proxy at all. This removes the casual path - an agent that would have PUT here because the endpoint existed and the token was lying on disk - and makes the refusal legible. The real boundary is normative. The new test drives all four mutating verbs with an admin-token principal, asserts the 403 and the code, and then asserts config.toml is byte-identical: a refusal that wrote something on the way to refusing is not a refusal. The route harness now passes an explicit principal, which is what surfaced this in the first place - 32 of its cases were silently exercising the untrusted path. The GUI is unaffected: gui/src/api.ts:95-109 authenticates with an ocx_session_ token, which resolves to gui-session. --- src/server/management/codex-prompt-routes.ts | 27 ++++++++++++++ tests/codex-prompt-route.test.ts | 39 ++++++++++++++++++-- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index b78717560e..0a70efe0c3 100644 --- a/src/server/management/codex-prompt-routes.ts +++ b/src/server/management/codex-prompt-routes.ts @@ -261,6 +261,33 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise { const url = new URL("http://127.0.0.1:10100" + pathname); const headers: Record = { host: "127.0.0.1:10100" }; @@ -88,7 +90,7 @@ async function call( try { res = await handleManagementAPI(req, url, config, { codexPromptPaths: { configPath: fx.configPath, storePath: fx.storePath }, - }); + }, principal); } finally { if (previousHome === undefined) delete process.env.CODEX_HOME; else process.env.CODEX_HOME = previousHome; @@ -583,7 +585,7 @@ describe("020 coverage completions", () => { }); const res = await handleManagementAPI(req, url, config, { codexPromptPaths: { configPath: fx.configPath, storePath: fx.storePath }, - }); + }, "gui-session"); expect(res!.status).toBe(400); } const url = new URL("http://127.0.0.1:10100/api/codex-prompt/adopt"); @@ -594,10 +596,41 @@ describe("020 coverage completions", () => { }); const res = await handleManagementAPI(req, url, config, { codexPromptPaths: { configPath: fx.configPath, storePath: fx.storePath }, - }); + }, "gui-session"); expect(res!.status).toBe(400); }); + test("an admin token can read the prompt stack but not rewrite it", async () => { + // The gate accepts the raw admin token before it consults the session table + // (management-auth.ts:462), and that token sits readable in ~/.opencodex. This + // endpoint writes the file that decides what the model reads, so the two + // credentials must not be interchangeable here. + // + // Reads stay open: describing the stack changes nothing, and the CLI parity + // path depends on it. + const fx = fixture("developer_instructions = \"Answer in Korean.\"\n"); + const readAsAdmin = await call("GET", "/api/codex-prompt", fx, undefined, "admin-token"); + expect(readAsAdmin.status).toBe(200); + const rev = readAsAdmin.body.revision as string; + const before = read(fx.configPath); + + // Every mutating verb, so a future route added to this file is covered by the + // same rule rather than needing its own test. + const writes: [string, string, unknown][] = [ + ["PUT", "/api/codex-prompt/toggle", { id: "permissions", enabled: false, revision: rev }], + ["PUT", "/api/codex-prompt/custom", { layers: [], revision: rev }], + ["POST", "/api/codex-prompt/adopt", { confirm: true, revision: rev }], + ["POST", "/api/codex-prompt/repair", { mode: "adopt", revision: rev }], + ]; + for (const [method, path, body] of writes) { + const res = await call(method, path, fx, body, "admin-token"); + expect(res.status).toBe(403); + expect(res.body.code).toBe("dashboard_session_required"); + } + // The refusal is not merely a status: nothing was written on the way to it. + expect(read(fx.configPath)).toBe(before); + }); + test("adopt refuses an oversized value, through BOTH import paths", async () => { // The owned-malformed repair branch reaches adoptDeveloperInstructions exactly // as /adopt does. Without a test on that branch, deleting its cap call is From 583f5a2717277902e478fac68ae3d5489f2e0b6c Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 01:51:31 +0900 Subject: [PATCH 3/6] fix(codex): keep a BOM at byte 0, and roll back a refused write Two ways the composer could damage a config file it had just reported writing. A UTF-8 BOM is only legal at byte 0, and setProjection inserted its two generated lines at line index 0 - ahead of the BOM. The write verified its own bytes and reported success; Codex could then no longer parse the file. Editors on Windows emit that byte routinely, so this was not an exotic input. The BOM is now split off before line editing and restored after, in all three editors that insert lines: the projection block and both boolean setters. The new tests parse the RESULT rather than asserting the bytes we meant to write. That distinction is the whole lesson here - the old write path was self-consistent and still produced an unloadable file. Bun.TOML is not the parser Codex uses, so a pass is not proof Codex accepts the file, but a failure is proof it does not, and that is the direction the assertion needs to hold in. Separately, only the CONFIG was checked for readability before the transaction began. An unwritable STORE - a directory on its path, a mode change, a full disk - threw out of durableWrite after the config had already been renamed into place. The exception escaped the transaction entirely, so rollback never ran: the caller saw a raw error, the config carried a projection whose store did not exist, and the orphaned journal made every later write fail recovery_required. The write steps are now wrapped, and a throw rolls back to the recorded pre-state and drops the journal. That failure gets its own error, write_failed, mapped to 500 rather than folded into write_superseded. The two are not the same: superseded means another writer won a race, while here nobody won and nothing landed, and retrying the same request unchanged will fail identically until the path or the disk is fixed. Both fixes confirmed by mutation. Removing the BOM handling turns exactly the projection test red; rethrowing instead of rolling back turns exactly the store test red. The two exhaustiveness guards on the write-error status map caught the new error on their own, which is what they were written for. --- src/codex/prompt-layers.ts | 92 ++++++++++++++----- src/server/management/codex-prompt-routes.ts | 4 + tests/codex-prompt-layers-write.test.ts | 93 +++++++++++++++++++- tests/codex-prompt-route.test.ts | 11 ++- 4 files changed, 175 insertions(+), 25 deletions(-) diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 7250ce80cf..301cb60b95 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -512,6 +512,10 @@ export type WriteError = | "store_unreadable" | "invalid_characters" | "write_superseded" + // The filesystem refused a rename that passed every precondition: a directory on + // the store path, a mode change, a full disk. Distinct from write_superseded, + // which means another writer won a race — here nobody won and nothing landed. + | "write_failed" | "recovery_required" | "locked"; @@ -531,6 +535,24 @@ function splitLines(content: string): string[] { return content.replace(/\r\n/g, "\n").split("\n"); } +/** + * A leading UTF-8 BOM, split off so line editing never steps over it. + * + * Codex reads config.toml with Rust `toml_edit`, which accepts a BOM at byte 0 and + * nowhere else. Inserting the generated block at line index 0 pushed the BOM down + * to byte 58, the write reported success because our own byte comparison matched + * what we intended to write, and the next parse failed with + * "Expected a key but found (0xEF)" — a config file the user could no longer load, + * produced by a write that told them it worked. + * + * Editors on Windows write this byte routinely, so the file is not exotic. + */ +function splitBom(content: string): { bom: string; body: string } { + return content.startsWith("\ufeff") + ? { bom: "\ufeff", body: content.slice(1) } + : { bom: "", body: content }; +} + function joinLines(lines: string[], eol: "\r\n" | "\n"): string { const text = lines.join("\n"); return eol === "\n" ? text : text.replace(/\n/g, "\r\n"); @@ -544,7 +566,8 @@ function firstTableIndex(lines: string[]): number { /** Set a root-scope boolean, inserting above the first table when absent. */ function setRootBool(content: string, key: string, value: boolean): string { const eol = dominantEol(content); - const lines = splitLines(content); + const { bom, body } = splitBom(content); + const lines = splitLines(body); const limit = firstTableIndex(lines); const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const pattern = new RegExp(`^(\\s*${escaped}\\s*=\\s*)(?:true|false)(\\s*(?:#.*)?)$`); @@ -552,23 +575,24 @@ function setRootBool(content: string, key: string, value: boolean): string { const m = pattern.exec(lines[i]!); if (m) { lines[i] = `${m[1]}${value}${m[2]}`; - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } } lines.splice(limit, 0, `${key} = ${value}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } /** Set a boolean inside `[table]`, appending the table when absent. */ function setTableBool(content: string, table: string, key: string, value: boolean): string { const eol = dominantEol(content); - const lines = splitLines(content); + const { bom, body } = splitBom(content); + const lines = splitLines(body); const escaped = table.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const start = lines.findIndex(l => new RegExp(`^\\s*\\[${escaped}\\]\\s*(?:#.*)?$`).test(l)); if (start === -1) { const tail = lines.length > 0 && lines[lines.length - 1] === "" ? lines.length - 1 : lines.length; lines.splice(tail, 0, `[${table}]`, `${key} = ${value}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } const keyEscaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const pattern = new RegExp(`^(\\s*${keyEscaped}\\s*=\\s*)(?:true|false)(\\s*(?:#.*)?)$`); @@ -578,11 +602,11 @@ function setTableBool(content: string, table: string, key: string, value: boolea const m = pattern.exec(lines[i]!); if (m) { lines[i] = `${m[1]}${value}${m[2]}`; - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } } lines.splice(end, 0, `${key} = ${value}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } /** @@ -593,7 +617,11 @@ function setTableBool(content: string, table: string, key: string, value: boolea function setProjection(content: string | null, projection: string | null): string { const base = content ?? ""; const eol = dominantEol(base); - const lines = splitLines(base); + // The BOM is held aside for the whole edit. This is the function that produced + // the corruption: the insert below is at index 0, which put the marker line + // ahead of a byte that is only legal at byte 0. + const { bom, body } = splitBom(base); + const lines = splitLines(body); const limit = firstTableIndex(lines); let markerAt = -1; @@ -607,12 +635,12 @@ function setProjection(content: string | null, projection: string | null): strin if (markerAt !== -1) { if (projection === null) lines.splice(markerAt, 2); else lines[markerAt + 1] = `${DEV_INSTRUCTIONS_KEY} = ${encodeBasicString(projection)}`; - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } - if (projection === null) return joinLines(lines, eol); + if (projection === null) return bom + joinLines(lines, eol); lines.splice(0, 0, OCX_SECTION_MARKER, `${DEV_INSTRUCTIONS_KEY} = ${encodeBasicString(projection)}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } function serializeStore(layers: readonly CustomLayer[]): string { @@ -691,19 +719,39 @@ function commit( // 4/5. each target re-verifies ITS OWN bytes immediately before its rename, // so a third party writing between step 2 and here is not overwritten. - if (configChanged) { - if (hashBytes(readFileOrNull(configPath)) !== record.preConfig) { - return rollback(record, journalPath, "stale_revision"); + // + // Wrapped, because a THROW here used to escape the transaction entirely. + // Only `config` readability is pre-checked, so an unwritable STORE — a + // directory sitting on its path, a permission change, a full disk — raised + // out of `durableWrite` after the config had already been renamed into + // place. The caller saw an exception, the config carried a projection whose + // store did not exist, and the journal stayed behind claiming an + // uncommitted intent. Every later write then failed recovery_required. + // + // Rolling back on the way out restores the pre-state we recorded and drops + // the journal, so a failed write leaves the pair exactly as it was found. + try { + if (configChanged) { + if (hashBytes(readFileOrNull(configPath)) !== record.preConfig) { + return rollback(record, journalPath, "stale_revision"); + } + if (nextConfig === null) durableDelete(configPath); + else durableWrite(configPath, nextConfig); } - if (nextConfig === null) durableDelete(configPath); - else durableWrite(configPath, nextConfig); - } - if (storeChanged) { - if (hashBytes(readFileOrNull(storePath)) !== record.preStore) { - return rollback(record, journalPath, "stale_revision"); + if (storeChanged) { + if (hashBytes(readFileOrNull(storePath)) !== record.preStore) { + return rollback(record, journalPath, "stale_revision"); + } + if (nextStore === null) durableDelete(storePath); + else durableWrite(storePath, nextStore); } - if (nextStore === null) durableDelete(storePath); - else durableWrite(storePath, nextStore); + } catch (error) { + // `rollback` is byte-hash driven and refuses to touch a file it does not + // recognise, so it is safe to run against a partially applied pair. If it + // cannot account for what it finds it returns recovery_required, which is the + // honest answer — better than a silent half-write either way. + const undone = rollback(record, journalPath, "write_failed"); + return { ...undone, detail: error instanceof Error ? error.message : String(error) } as WriteResult; } // 6. verify COMPLETE bytes, not just our two lines: another writer could diff --git a/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index 0a70efe0c3..21968e6947 100644 --- a/src/server/management/codex-prompt-routes.ts +++ b/src/server/management/codex-prompt-routes.ts @@ -72,6 +72,10 @@ const WRITE_ERROR_STATUS: Record = { store_unreadable: 409, invalid_characters: 400, write_superseded: 409, + // Not the caller's fault and not a race: the filesystem refused the write and the + // transaction rolled itself back. 500 rather than 409 — retrying the same request + // unchanged will fail the same way until the disk or the path is fixed. + write_failed: 500, recovery_required: 409, locked: 409, }; diff --git a/tests/codex-prompt-layers-write.test.ts b/tests/codex-prompt-layers-write.test.ts index 97f37e2930..b05d686892 100644 --- a/tests/codex-prompt-layers-write.test.ts +++ b/tests/codex-prompt-layers-write.test.ts @@ -4,7 +4,7 @@ * Explicit temp paths only — these functions write a user's live Codex config. */ import { afterEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -265,4 +265,95 @@ describe("transaction", () => { setToggle("apps", false, first, paths); expect(readPromptLayers(paths).revision).not.toBe(first); }); + + // BUG-R2: a BOM-prefixed config was corrupted by an insert at line 0. + // + // Each of these parses the RESULT. Asserting the bytes we meant to write is what + // let the defect ship: the write verified its own intent and the file it produced + // could not be loaded. Bun.TOML is not what Codex uses, so a pass here is not + // proof Codex accepts the file - but a FAILURE is proof it does not, and that is + // the direction this assertion needs to be sound in. + describe("a UTF-8 BOM survives every write", () => { + const BOM = "\ufeff"; + + test("the projection insert keeps the BOM at byte 0", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const snap = readPromptLayers(paths); + const result = writeCustomLayers([layer()], snap.revision, paths); + expect(result.ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.startsWith(BOM)).toBe(true); + expect(after.indexOf(BOM)).toBe(0); + // Exactly one: a second BOM mid-document is as unparseable as a displaced one. + expect(after.split(BOM).length - 1).toBe(1); + expect(after).toContain(MARKER); + expect(() => Bun.TOML.parse(after)).not.toThrow(); + }); + + test("a root toggle keeps the BOM at byte 0", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const snap = readPromptLayers(paths); + expect(setToggle("apps", false, snap.revision, paths).ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.indexOf(BOM)).toBe(0); + expect(after.split(BOM).length - 1).toBe(1); + expect(Bun.TOML.parse(after)).toMatchObject({ include_apps_instructions: false }); + }); + + test("a table toggle keeps the BOM at byte 0", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const snap = readPromptLayers(paths); + expect(setToggle("skills", false, snap.revision, paths).ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.indexOf(BOM)).toBe(0); + expect(Bun.TOML.parse(after)).toMatchObject({ skills: { include_instructions: false } }); + }); + + test("removing the projection does not leave the BOM behind", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const added = writeCustomLayers([layer()], readPromptLayers(paths).revision, paths); + expect(added.ok).toBe(true); + const removed = writeCustomLayers([], readPromptLayers(paths).revision, paths); + expect(removed.ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.indexOf(BOM)).toBe(0); + expect(after).not.toContain(MARKER); + expect(() => Bun.TOML.parse(after)).not.toThrow(); + }); + + test("a file with no BOM does not gain one", () => { + const paths = fixture("model = \"x\"\n"); + expect(writeCustomLayers([layer()], readPromptLayers(paths).revision, paths).ok).toBe(true); + expect(read(paths.configPath)!).not.toContain(BOM); + }); + }); + + // BUG-R3: an unwritable store left config.toml mutated and the journal orphaned. + test("a store the filesystem refuses rolls the config back", () => { + const paths = fixture("model = \"x\"\n"); + // A DIRECTORY on the store path. Only config readability was pre-checked, so + // durableWrite threw here AFTER the config had already been renamed into place - + // and the throw escaped the transaction, skipping rollback entirely. + mkdirSync(paths.storePath, { recursive: true }); + const before = read(paths.configPath); + + const result = writeCustomLayers([layer()], readPromptLayers(paths).revision, paths); + + expect(result.ok).toBe(false); + if (result.ok) throw new Error("unreachable"); + expect(result.error).toBe("write_failed"); + // The three things the old behaviour got wrong, asserted separately because each + // one is independently damaging. + expect(read(paths.configPath)).toBe(before); + expect(existsSync(join(paths.root, "opencodex-prompt.journal"))).toBe(false); + expect(existsSync(join(paths.root, "opencodex-prompt.lock"))).toBe(false); + + // And the next write is not poisoned by the failed one. + rmSync(paths.storePath, { recursive: true, force: true }); + expect(writeCustomLayers([layer()], readPromptLayers(paths).revision, paths).ok).toBe(true); + }); }); diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index b223754612..d1faf27699 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -438,9 +438,15 @@ describe("dispatch and safety", () => { // typecheck property of Record. This asserts the values. const { WRITE_ERROR_STATUS_FOR_TESTS } = await import("../src/server/management/codex-prompt-routes"); const statuses = Object.values(WRITE_ERROR_STATUS_FOR_TESTS); - expect(statuses.length).toBeGreaterThanOrEqual(9); + expect(statuses.length).toBeGreaterThanOrEqual(10); for (const status of statuses) expect(status).toBeGreaterThanOrEqual(400); - for (const status of statuses) expect(status).toBeLessThan(500); + // write_failed is the one 5xx: the filesystem refused a write that passed every + // precondition, so the caller did nothing wrong and retrying it unchanged will + // fail identically. Every OTHER error stays 4xx. + for (const [error, status] of Object.entries(WRITE_ERROR_STATUS_FOR_TESTS)) { + if (error === "write_failed") expect(status).toBe(500); + else expect(status).toBeLessThan(500); + } }); test("22. the injected paths are honored on every verb", async () => { @@ -567,6 +573,7 @@ describe("020 coverage completions", () => { store_unreadable: 409, invalid_characters: 400, write_superseded: 409, + write_failed: 500, recovery_required: 409, locked: 409, }); From 59d9bc95f197635e48544a77f732782fb98f1701 Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 02:12:09 +0900 Subject: [PATCH 4/6] fix: stop deleting reused branches, and finish the glm-5.3-flash correction Three more findings from the dev-versus-main review. The closed-PR branch cleanup matched branches by NAME alone. Any branch whose same-name historical pull requests were all closed became a deletion candidate, without checking that the branch still pointed at one of their head commits. A `codex/`-style name reused for new work inherited the closed history of every PR that had ever carried that label, and the commits it carried had never been in a pull request at all. The planner now requires the current tip to match a closed head SHA, and keeps the branch when either SHA is unknown. The script had no test of any kind, which is how this shipped; it has eight now, including one per existing safety rule so the tip check cannot be reordered ahead of them. glm-5.3-flash was still routed through the vision sidecar on eight providers. The correction commit fixed the two Alibaba entries and left umans, cline-pass, nvidia, zai, zhipu-bigmodel-coding, both Volcengine plans and ollama-cloud behind. That list means "the proxy describes this model's images", so a native VLM sitting in it got a text description of a picture it could read itself: no error, a worse answer, an extra call. The parity assertion is now written across the whole registry rather than per provider, because the defect was entries drifting apart rather than one being wrong. It was also missing from ZAI_GLM_53_MODELS, so on Z.AI it advertised a 1M context with a null effort ladder, no default effort and no output cap while its siblings carried three tiers, a max default and 131072 tokens. The parity test pinned that gap in place because it was written from the incomplete state instead of from the family definition. And the Moonshot `$ref` normalizer overwrote numeric bounds with the sibling's instead of intersecting them. Its comment claimed the node "narrows the target", which is true only when the node happens to be narrower - a node declaring minLength 1 beside a target declaring minLength 5 emitted minLength 1, a contract weaker than either side asked for. That is the same failure the required/properties composition already fixed for set-valued keywords. Lower bounds now take the max, upper bounds the min, and a test covers both directions so the fix cannot become "always prefer the target", which would discard a real narrowing. Each fix is mutation-verified: disabling the tip comparison turns the reused-branch test red on its own, and the looser-bound test was red before the intersection landed and green after. --- .github/scripts/closed-pr-branch-cleanup.cjs | 74 +++++++- .../workflows/cleanup-closed-pr-branches.yml | 9 +- src/adapters/openai-chat.ts | 50 ++++++ src/providers/registry.ts | 58 ++++++- tests/closed-pr-branch-cleanup.test.ts | 160 ++++++++++++++++++ tests/moonshot-tool-schema.test.ts | 60 +++++++ tests/provider-registry-parity.test.ts | 38 ++++- 7 files changed, 433 insertions(+), 16 deletions(-) create mode 100644 tests/closed-pr-branch-cleanup.test.ts diff --git a/.github/scripts/closed-pr-branch-cleanup.cjs b/.github/scripts/closed-pr-branch-cleanup.cjs index aaca0f6498..57142f19fe 100644 --- a/.github/scripts/closed-pr-branch-cleanup.cjs +++ b/.github/scripts/closed-pr-branch-cleanup.cjs @@ -23,6 +23,20 @@ function normalizeBranchName(value) { return String(value || "").trim(); } +/** + * A commit id, lowercased for comparison. + * + * The REST and GraphQL APIs are not consistent about case, and a full 40-character + * sha compared case-sensitively against an abbreviated or upper-case one silently + * reads as "different" - which here would mean "keep", so the failure direction is + * safe, but it would make the guard useless rather than protective. Anything that + * is not a plausible hex object id becomes null, i.e. unknown. + */ +function normalizeOid(value) { + const text = String(value || "").trim().toLowerCase(); + return /^[0-9a-f]{7,64}$/.test(text) ? text : null; +} + function isProtectedBranch(name) { return PROTECTED_BRANCHES.includes(normalizeBranchName(name)); } @@ -45,6 +59,8 @@ const KEEP_REASONS = Object.freeze({ CROSS_REPOSITORY: "cross-repository-head", MISSING_CLOSED_AT: "missing-closed-at", WITHIN_GRACE: "within-grace-period", + MOVED_SINCE_CLOSE: "branch-moved-since-close", + UNKNOWN_HEAD_SHA: "unknown-head-sha", }); /** @@ -63,12 +79,23 @@ const KEEP_REASONS = Object.freeze({ * contributor's repository and this token has no business there. * - A grace period after `closed_at` leaves room to reopen a PR that was * closed by mistake. + * - The branch must still POINT AT a commit one of those closed pull requests + * had as its head. Matching by NAME alone deletes reused work: `codex/`-style + * names get picked up again all the time, and a branch recreated for new work + * inherits the closed history of every PR that ever used that name. The tip + * moved, so the branch is not the closed PR's branch any more - it only shares + * its label. + * - A branch whose current tip cannot be determined is kept. An unknown tip is + * not evidence of an abandoned branch, and this job's mistakes are not + * recoverable. * * @param {object} input * @param {Array} input.pullRequests Pull requests with - * `headRefName`, `baseRefName`, `state`, `merged`, `closedAt`, and - * `isCrossRepository`. - * @param {Array} input.branches Branch names that currently exist. + * `headRefName`, `headRefOid`, `baseRefName`, `state`, `merged`, `closedAt`, + * and `isCrossRepository`. + * @param {Array} input.branches Branches + * that currently exist. A bare string carries no tip, which is treated as an + * unknown tip and kept. * @param {number} [input.now] Current time in milliseconds. * @param {number} [input.graceDays] Days to wait after `closedAt`. * @returns {{ deletions: Array<{branch: string, pullRequests: number[]}>, @@ -80,7 +107,17 @@ function planClosedPrBranchDeletions({ now = Date.now(), graceDays = DEFAULT_GRACE_DAYS, }) { - const existing = new Set(branches.map(normalizeBranchName).filter(Boolean)); + // Accepts both shapes so an older caller passing bare names still works - it + // just gets the conservative answer, because a name without a tip cannot be + // proven safe to delete. + /** @type {Map} */ + const existing = new Map(); + for (const entry of branches) { + const name = normalizeBranchName(typeof entry === "string" ? entry : entry && entry.name); + if (!name) continue; + const oid = typeof entry === "string" ? null : normalizeOid(entry && entry.oid); + existing.set(name, oid); + } const graceMs = Math.max(0, Number(graceDays) || 0) * 24 * 60 * 60 * 1000; /** @type {Map} */ @@ -104,7 +141,7 @@ function planClosedPrBranchDeletions({ const deletions = []; const keeps = []; - for (const branch of [...existing].sort()) { + for (const branch of [...existing.keys()].sort()) { if (isProtectedBranch(branch)) { keeps.push({ branch, reason: KEEP_REASONS.PROTECTED }); continue; @@ -141,6 +178,33 @@ function planClosedPrBranchDeletions({ continue; } + // The tip check, last because it is the most expensive claim to satisfy and + // the cheaper rules above have already excluded most branches. + // + // A closed PR's head branch is only THIS branch if the branch still points at + // a commit that PR had as its head. Without this, a name reused for new work + // is deleted on the strength of an unrelated PR that happened to share the + // label months earlier - and a deleted branch whose commits were never pushed + // anywhere else is gone. + const currentOid = existing.get(branch) || null; + if (!currentOid) { + keeps.push({ branch, reason: KEEP_REASONS.UNKNOWN_HEAD_SHA }); + continue; + } + const closedOids = new Set( + related.map((pr) => normalizeOid(pr && pr.headRefOid)).filter(Boolean), + ); + // An empty set means the API gave us no head SHA for any of them, which is the + // unknown case again rather than a licence to delete. + if (closedOids.size === 0) { + keeps.push({ branch, reason: KEEP_REASONS.UNKNOWN_HEAD_SHA }); + continue; + } + if (!closedOids.has(currentOid)) { + keeps.push({ branch, reason: KEEP_REASONS.MOVED_SINCE_CLOSE }); + continue; + } + deletions.push({ branch, pullRequests: related diff --git a/.github/workflows/cleanup-closed-pr-branches.yml b/.github/workflows/cleanup-closed-pr-branches.yml index c260bc03a8..4b0c1229a9 100644 --- a/.github/workflows/cleanup-closed-pr-branches.yml +++ b/.github/workflows/cleanup-closed-pr-branches.yml @@ -70,6 +70,10 @@ jobs: merged: Boolean(pr.merged_at), closedAt: pr.closed_at, headRefName: pr.head && pr.head.ref, + // The tip this PR actually pointed at. Without it the planner cannot + // tell a genuinely abandoned branch from a name someone reused, and + // keeps the branch instead of deleting it. + headRefOid: pr.head && pr.head.sha, baseRefName: pr.base && pr.base.ref, // A fork head lives in the contributor's repository. Comparing // repo ids (not names) keeps a same-name fork from looking local. @@ -82,7 +86,10 @@ jobs: repo, per_page: 100, }); - const branches = rawBranches.map((branch) => branch.name); + const branches = rawBranches.map((branch) => ({ + name: branch.name, + oid: branch.commit && branch.commit.sha, + })); const protectedByGitHub = new Set( rawBranches.filter((branch) => branch.protected).map((branch) => branch.name), ); diff --git a/src/adapters/openai-chat.ts b/src/adapters/openai-chat.ts index cc8dcaad89..417f4ede4e 100644 --- a/src/adapters/openai-chat.ts +++ b/src/adapters/openai-chat.ts @@ -1041,6 +1041,48 @@ function unionRequired(target: unknown, sibling: unknown): unknown { */ const MOONSHOT_DATA_VALUED_KEYWORDS = new Set(["enum", "const", "default", "examples"]); +/** + * Numeric assertions whose intersection is a bound, and which direction tightens. + * + * `$ref` under 2020-12 is an in-place applicator: the node and its target BOTH apply, so + * the emitted schema must be their INTERSECTION. The previous code overwrote the target + * with the node and called that "the narrower reading", which holds only when the node + * happens to be narrower. A node declaring `minLength: 1` beside a target declaring + * `minLength: 5` shipped `minLength: 1` - a contract weaker than either side asked for, + * emitted silently, which is the same failure mode the `required` composition fixed for + * set-valued keywords. + * + * "max" means the surviving value is the larger of the two (lower bounds), "min" the + * smaller (upper bounds). A keyword absent from this table keeps the overwrite: for + * `type`, `format`, `description` and friends there is no ordering to intersect along, + * and the node is the more specific statement. + */ +const MOONSHOT_BOUND_KEYWORDS: Record = { + minLength: "max", + minItems: "max", + minProperties: "max", + minimum: "max", + exclusiveMinimum: "max", + maxLength: "min", + maxItems: "min", + maxProperties: "min", + maximum: "min", + exclusiveMaximum: "min", +}; + +/** + * Intersect one numeric bound. Either side being absent or non-finite yields the other, + * because an unstated bound constrains nothing - returning `undefined` there would drop + * a constraint the remaining side genuinely made. + */ +function intersectBound(target: unknown, sibling: unknown, direction: "max" | "min"): unknown { + const a = typeof target === "number" && Number.isFinite(target) ? target : null; + const b = typeof sibling === "number" && Number.isFinite(sibling) ? sibling : null; + if (a === null) return b === null ? sibling : sibling; + if (b === null) return target; + return direction === "max" ? Math.max(a, b) : Math.min(a, b); +} + /** * Compose two `properties` maps. A property named in BOTH the referenced target and the * node is the same conjunction problem `required` had: letting the sibling win discards @@ -1130,6 +1172,14 @@ function normalizeMoonshotSchemaNode( merged[key] = composeProperties(merged[key] as Record, normalized); continue; } + // Numeric bounds intersect rather than overwrite: both the node and its target + // apply, so the surviving bound is the stricter of the two in whichever direction + // that keyword tightens. + const boundDirection = MOONSHOT_BOUND_KEYWORDS[key]; + if (boundDirection && key in merged) { + merged[key] = intersectBound(merged[key], normalized, boundDirection); + continue; + } merged[key] = normalized; } return merged; diff --git a/src/providers/registry.ts b/src/providers/registry.ts index 40feec0bfd..d15814d830 100644 --- a/src/providers/registry.ts +++ b/src/providers/registry.ts @@ -340,9 +340,31 @@ const ANTHROPIC_MODEL_CONTEXT_WINDOWS: Record = { "claude-sonnet // The non-Z.AI providers below are speculative on purpose: they carry 5.2 today and are // expected to pick 5.3 up on their usual lag. Providers whose live /v1/models discovery is // enabled self-correct on the next successful fetch; static ones need a follow-up refresh. -const ZAI_GLM_53_MODELS = ["glm-5.3", "glm-5.3[1m]"]; +// Every 5.3 family member, so the effort ladder, the default effort and the output +// cap are derived in ONE place. `glm-5.3-flash` was seeded into the model list and +// the context map by hand and left out of this constant, which meant it advertised +// a 1M context with a null effort ladder, no default effort and no output cap while +// its siblings carried three tiers, a `max` default and 131072 tokens. A member +// added to the list but not to the family is a model whose metadata silently +// disappears. +const ZAI_GLM_53_MODELS = ["glm-5.3", "glm-5.3[1m]", "glm-5.3-flash"]; const ZAI_GLM_52_MODELS = ["glm-5.2", "glm-5.2[1m]"]; const ZAI_GLM_5X_MODELS = [...ZAI_GLM_53_MODELS, ...ZAI_GLM_52_MODELS]; +/** + * The 5.x rows whose images the PROXY has to describe, which is NOT the same set as + * the 5.x rows themselves. + * + * `glm-5.3-flash` is a native VLM (docs.z.ai/guides/vlm/glm-5.3-flash), so listing it + * in `noVisionModels` sent an image through the vision sidecar and handed the model a + * text description of a picture it could have read itself - no error, worse answer, + * extra call. The correction commit fixed the Alibaba entries and left the eight + * providers that reach this constant behind. + * + * Kept separate from ZAI_GLM_5X_MODELS rather than filtered at each use site: that + * constant also drives `modelSupportsReasoningSummaries` and + * `preserveReasoningContentModels`, where flash DOES belong. + */ +const ZAI_GLM_5X_SIDECAR_VISION_MODELS = ZAI_GLM_5X_MODELS.filter(id => id !== "glm-5.3-flash"); const ZAI_GLM_52_REASONING_EFFORTS = ["low", "medium", "high", "xhigh", "max"]; /** * GLM-5.3 does NOT share 5.2's five-tier ladder. docs.z.ai/devpack/latest-model folds every @@ -471,7 +493,9 @@ const OPENCODE_GO_THINKING_TOGGLE_MODELS = [ * images through the proxy's vision sidecar (src/codex/catalog/provider-fetch.ts), a claim nobody * has verified for BigModel-hosted GLM. */ -const ZHIPU_BIGMODEL_TEXT_MODELS = ["glm-4.6", "glm-4.7", "glm-4.7-flash", "glm-5", "glm-5.1", "glm-5.2", "glm-5.3", "glm-5.3-flash"]; +// `glm-5.3-flash` is deliberately absent: it is a native VLM +// (docs.z.ai/guides/vlm/glm-5.3-flash), unlike glm-5.3 itself. +const ZHIPU_BIGMODEL_TEXT_MODELS = ["glm-4.6", "glm-4.7", "glm-4.7-flash", "glm-5", "glm-5.1", "glm-5.2", "glm-5.3"]; const ZHIPU_BIGMODEL_MODELS = [...ZHIPU_BIGMODEL_TEXT_MODELS, "glm-4.6v"]; const ZHIPU_BIGMODEL_INPUT_MODALITIES: Record = { ...Object.fromEntries(ZHIPU_BIGMODEL_TEXT_MODELS.map(id => [id, ["text"]])), @@ -694,6 +718,9 @@ const VOLCENGINE_AGENT_PLAN_MODELS = [ const VOLCENGINE_PLAN_INPUT_MODALITIES: Record = { "kimi-k2.6": ["text", "image"], "minimax-m3": ["text", "image"], + // Native VLM (docs.z.ai/guides/vlm/glm-5.3-flash), so it is declared here and left + // out of the text-only list below. + "glm-5.3-flash": ["text", "image"], }; // Every other Plan model is text-only. Declaring this explicitly keeps the vision // sidecar from advertising image input for models that cannot accept it — the same @@ -704,7 +731,6 @@ const VOLCENGINE_PLAN_TEXT_ONLY_MODELS = [ "deepseek-v4-pro", "deepseek-v4-flash", "glm-5.3", - "glm-5.3-flash", "glm-5.2", "doubao-seed-2.0-pro", ]; @@ -812,6 +838,7 @@ const NVIDIA_NIM_VISION_MODELS = [ "minimaxai/minimax-m3", "moonshotai/kimi-k2.6", "moonshotai/kimi-k2.5", "stepfun-ai/step-3.7-flash", "thinkingmachines/inkling", "mistralai/mistral-medium-3.5-128b", + "z-ai/glm-5.3-flash", ]; /** * The catalog advertises image input only for `noVisionModels` members, so a natively @@ -845,7 +872,11 @@ const NVIDIA_NIM_NO_VISION_MODELS = [ "nvidia/nemotron-3-ultra-550b-a55b", "nvidia/nemotron-mini-4b-instruct", "nvidia/nvidia-nemotron-nano-9b-v2", "openai/gpt-oss-120b", "openai/gpt-oss-20b", - "poolside/laguna-xs-2.1", "z-ai/glm-5.3", "z-ai/glm-5.3-flash", "z-ai/glm-5.2", + // z-ai/glm-5.3-flash belongs in NVIDIA_NIM_VISION_MODELS, not here: Z.AI documents + // it under docs.z.ai/guides/vlm/. The header above says an id must be classified + // deliberately rather than assumed from its name, and inheriting glm-5.3's + // text-only verdict because of the shared prefix is exactly that mistake. + "poolside/laguna-xs-2.1", "z-ai/glm-5.3", "z-ai/glm-5.2", ]; const KIMI_CODING_MODEL_CONTEXT_WINDOWS: Record = Object.fromEntries( KIMI_CODING_MODELS.map(id => [id, id === "k3[1m]" ? KIMI_K3_1M_CONTEXT_WINDOW : KIMI_K3_STANDARD_CONTEXT_WINDOW]), @@ -976,7 +1007,11 @@ const UMANS_GLM_REASONING_EFFORTS = ["high", "xhigh", "max"]; // 260814: Z.AI folds GLM-5.3 efforts into low/high/max, so `low` is a real tier here and // `xhigh` is not distinct from `max` (docs.z.ai/devpack/latest-model). const UMANS_GLM_53_REASONING_EFFORTS = ["low", "high", "max"]; -const UMANS_TEXT_ONLY_MODELS = ["umans-glm-5.3", "umans-glm-5.3-flash", "umans-glm-5.2", "umans-glm-5.1"]; +// `umans-glm-5.3-flash` is NOT here: Z.AI documents glm-5.3-flash under +// docs.z.ai/guides/vlm/, so it takes images natively and does not need the proxy's +// vision sidecar. The seeding pass classified it from the family name and a later +// pass corrected only some of the providers; this is one it missed. +const UMANS_TEXT_ONLY_MODELS = ["umans-glm-5.3", "umans-glm-5.2", "umans-glm-5.1"]; const UMANS_MODEL_CONTEXT_WINDOWS: Record = { "umans-coder": 262_144, "umans-kimi-k2.7": 262_144, @@ -1030,6 +1065,11 @@ const CLINE_PASS_IMAGE_MODELS = new Set([ "cline-pass/mimo-v2.5", "cline-pass/minimax-m3", "cline-pass/qwen3.7-plus", + // Native VLM (docs.z.ai/guides/vlm/), so its images do not go through the proxy's + // sidecar. Adding it here moves it out of CLINE_PASS_TEXT_ONLY_MODELS and flips its + // declared modalities to ["text", "image"] in one edit, because both are derived + // from this set. + "cline-pass/glm-5.3-flash", ]); const CLINE_PASS_MODALITY_KNOWN_MODELS = CLINE_PASS_MODELS.filter(id => id !== "cline-pass/qwen3.8-max"); const CLINE_PASS_TEXT_ONLY_MODELS = CLINE_PASS_MODALITY_KNOWN_MODELS.filter(id => !CLINE_PASS_IMAGE_MODELS.has(id)); @@ -2199,7 +2239,7 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ modelContextWindows: { "glm-5.3": 1_000_000, "glm-5.3[1m]": 1_000_000, "glm-5.3-flash": 1_000_000, "glm-5.2": 1_000_000, "glm-5.2[1m]": 1_000_000 }, // Z.AI's OpenAI path returns 400 code 1211 for bracketed model ids. modelSuffixBracketStrip: true, - noVisionModels: ZAI_GLM_5X_MODELS, + noVisionModels: ZAI_GLM_5X_SIDECAR_VISION_MODELS, modelReasoningEfforts: ZAI_GLM_5X_REASONING_EFFORTS, modelDefaultReasoningEfforts: Object.fromEntries(ZAI_GLM_53_MODELS.map(id => [id, "max"])), modelMaxOutputTokens: Object.fromEntries(ZAI_GLM_53_MODELS.map(id => [id, 131_072])), @@ -2280,7 +2320,7 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ jawcodeBundle: "zai", modelContextWindows: { "glm-5.3": 1_000_000, "glm-5.3[1m]": 1_000_000, "glm-5.3-flash": 1_000_000, "glm-5.2": 1_000_000, "glm-5.2[1m]": 1_000_000 }, modelSuffixBracketStrip: true, - noVisionModels: ZAI_GLM_5X_MODELS, + noVisionModels: ZAI_GLM_5X_SIDECAR_VISION_MODELS, modelReasoningEfforts: ZAI_GLM_5X_REASONING_EFFORTS, modelSupportsReasoningSummaries: Object.fromEntries(ZAI_GLM_5X_MODELS.map(id => [id, true])), preserveReasoningContentModels: ZAI_GLM_5X_MODELS, @@ -2513,7 +2553,9 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ models: ["glm-5.3", "glm-5.3-flash", "glm-5.2", "deepseek-v4-pro", "qwen3-coder:480b", "gpt-oss:120b", "kimi-k2.6", "minimax-m3", "qwen3.5:397b", "gemma4:31b"], defaultModel: "glm-5.3", noVisionModels: [ - "glm-5.3", "glm-5.3-flash", "glm-5.2", "glm-5.1", "glm-5", "glm-4.7", + // glm-5.3-flash is absent on purpose: native VLM + // (docs.z.ai/guides/vlm/glm-5.3-flash), so its images skip the sidecar. + "glm-5.3", "glm-5.2", "glm-5.1", "glm-5", "glm-4.7", "minimax-m2.7", "minimax-m2.5", "minimax-m2.1", "nemotron-3-ultra", "nemotron-3-super", "deepseek-v4-pro", "deepseek-v4-flash", diff --git a/tests/closed-pr-branch-cleanup.test.ts b/tests/closed-pr-branch-cleanup.test.ts new file mode 100644 index 0000000000..63cd46f204 --- /dev/null +++ b/tests/closed-pr-branch-cleanup.test.ts @@ -0,0 +1,160 @@ +/** + * Deletion planning for .github/scripts/closed-pr-branch-cleanup.cjs. + * + * This job deletes branches, so every test here is a safety test. It had no + * coverage at all, which is how a name-only match reached main: the planner + * selected any branch whose same-NAME historical pull requests were all closed, + * without checking that the branch still pointed at one of their head commits. + * A `codex/`-style name reused for new work inherited the closed history of + * every PR that had ever carried that label. + */ +import { describe, expect, test } from "bun:test"; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +const cleanup = require("../.github/scripts/closed-pr-branch-cleanup.cjs") as { + DEFAULT_GRACE_DAYS: number; + KEEP_REASONS: Record; + PROTECTED_BRANCHES: string[]; + isProtectedBranch: (name: string) => boolean; + planClosedPrBranchDeletions: (input: { + pullRequests?: unknown[]; + branches?: unknown[]; + now?: number; + graceDays?: number; + }) => { deletions: { branch: string; pullRequests: number[] }[]; keeps: { branch: string; reason: string }[] }; +}; + +const { KEEP_REASONS, planClosedPrBranchDeletions } = cleanup; + +const NOW = Date.parse("2026-08-27T00:00:00Z"); +const LONG_AGO = new Date(NOW - 90 * 24 * 60 * 60 * 1000).toISOString(); +const OLD_TIP = "a".repeat(40); +const NEW_TIP = "b".repeat(40); + +function closedPr(over: Record = {}) { + return { + number: 42, + state: "CLOSED", + merged: false, + closedAt: LONG_AGO, + headRefName: "codex/some-work", + headRefOid: OLD_TIP, + baseRefName: "dev", + isCrossRepository: false, + ...over, + }; +} + +function plan(pullRequests: unknown[], branches: unknown[]) { + return planClosedPrBranchDeletions({ pullRequests, branches, now: NOW, graceDays: 14 }); +} + +function keepReason(result: ReturnType, branch: string): string | undefined { + return result.keeps.find(k => k.branch === branch)?.reason; +} + +describe("closed-PR branch cleanup planning", () => { + test("an abandoned branch still at the closed PR tip is deleted", () => { + // The case the job exists for. If this stops passing the job has become + // a no-op, which is a different failure from deleting live work but still + // a failure. + const result = plan( + [closedPr()], + [{ name: "codex/some-work", oid: OLD_TIP }], + ); + expect(result.deletions).toEqual([{ branch: "codex/some-work", pullRequests: [42] }]); + }); + + test("BUG-R4: a branch reused for new work is kept, not deleted", () => { + // Same NAME, different tip. Before the SHA guard this returned a deletion + // for a branch carrying commits that had never been in any pull request. + const result = plan( + [closedPr()], + [{ name: "codex/some-work", oid: NEW_TIP }], + ); + expect(result.deletions).toEqual([]); + expect(keepReason(result, "codex/some-work")).toBe(KEEP_REASONS.MOVED_SINCE_CLOSE); + }); + + test("a tip matching ANY of several closed PRs is enough", () => { + // Reopening and reclosing a branch, or two PRs from the same head, must not + // make the branch undeletable forever - matching one closed head is the bar. + const result = plan( + [ + closedPr({ number: 7, headRefOid: OLD_TIP }), + closedPr({ number: 9, headRefOid: NEW_TIP }), + ], + [{ name: "codex/some-work", oid: NEW_TIP }], + ); + expect(result.deletions).toEqual([{ branch: "codex/some-work", pullRequests: [7, 9] }]); + }); + + test("an unknown current tip is kept", () => { + // A bare string carries no tip. An older caller passing names gets the + // conservative answer rather than the old destructive one. + const result = plan([closedPr()], ["codex/some-work"]); + expect(result.deletions).toEqual([]); + expect(keepReason(result, "codex/some-work")).toBe(KEEP_REASONS.UNKNOWN_HEAD_SHA); + }); + + test("an unknown closed head SHA is kept", () => { + const result = plan( + [closedPr({ headRefOid: null })], + [{ name: "codex/some-work", oid: OLD_TIP }], + ); + expect(result.deletions).toEqual([]); + expect(keepReason(result, "codex/some-work")).toBe(KEEP_REASONS.UNKNOWN_HEAD_SHA); + }); + + test("SHA comparison ignores case", () => { + // The REST and GraphQL APIs disagree about case. A case-sensitive compare + // would keep every branch and quietly turn the job into a no-op. + const result = plan( + [closedPr({ headRefOid: OLD_TIP.toUpperCase() })], + [{ name: "codex/some-work", oid: OLD_TIP }], + ); + expect(result.deletions).toHaveLength(1); + }); + + test("the existing safety rules still hold ahead of the tip check", () => { + // Each of these must win BEFORE the SHA comparison, so a matching tip cannot + // override them. Asserted through the keep reason, not just the empty + // deletion list: the reason is what proves which rule fired. + const at = (name: string, oid: string | null = OLD_TIP) => [{ name, oid }]; + + const merged = plan([closedPr({ merged: true })], at("codex/some-work")); + expect(keepReason(merged, "codex/some-work")).toBe(KEEP_REASONS.MERGED); + + const open = plan([closedPr({ state: "OPEN" })], at("codex/some-work")); + expect(keepReason(open, "codex/some-work")).toBe(KEEP_REASONS.OPEN); + + const fork = plan([closedPr({ isCrossRepository: true })], at("codex/some-work")); + expect(keepReason(fork, "codex/some-work")).toBe(KEEP_REASONS.CROSS_REPOSITORY); + + const stacked = plan( + [ + closedPr(), + closedPr({ number: 43, state: "OPEN", headRefName: "codex/child", baseRefName: "codex/some-work" }), + ], + at("codex/some-work"), + ); + expect(keepReason(stacked, "codex/some-work")).toBe(KEEP_REASONS.BASE_OF_OPEN); + + const recent = plan( + [closedPr({ closedAt: new Date(NOW - 60 * 60 * 1000).toISOString() })], + at("codex/some-work"), + ); + expect(keepReason(recent, "codex/some-work")).toBe(KEEP_REASONS.WITHIN_GRACE); + + const protectedBranch = plan([closedPr({ headRefName: "dev" })], at("dev")); + expect(keepReason(protectedBranch, "dev")).toBe(KEEP_REASONS.PROTECTED); + }); + + test("a branch no pull request ever used is out of scope entirely", () => { + // Neither deleted nor reported as a keep: this job only speaks about + // branches it can attribute to a pull request. + const result = plan([closedPr()], [{ name: "codex/never-a-pr", oid: NEW_TIP }]); + expect(result.deletions).toEqual([]); + expect(keepReason(result, "codex/never-a-pr")).toBeUndefined(); + }); +}); diff --git a/tests/moonshot-tool-schema.test.ts b/tests/moonshot-tool-schema.test.ts index 80ceb5e5b9..ae37643172 100644 --- a/tests/moonshot-tool-schema.test.ts +++ b/tests/moonshot-tool-schema.test.ts @@ -226,6 +226,66 @@ describe("Moonshot tool schema normalization (issue #2673)", () => { expect(value.enum).toEqual(["x", "y"]); }); + // BUG-R6: "the node narrows the target" was asserted, never enforced. + // + // The test above uses a node whose minLength is TIGHTER than the target's, so a plain + // overwrite and a real narrowing are indistinguishable there. When the node is LOOSER, + // the two diverge and the overwrite ships the weaker contract - the opposite of what + // the comment claims and of what `$ref` means under 2020-12, where the node and its + // target both apply. + test("a looser sibling assertion does not relax the target", async () => { + const parameters = await emittedParameters("https://api.moonshot.ai/v1", { + name: "loosening_tool", + parameters: { + type: "object", + $defs: { + Tight: { + type: "string", + minLength: 5, + maxLength: 10, + minimum: 10, + maximum: 100, + }, + }, + properties: { + value: { + $ref: "#/$defs/Tight", + // Every one of these is weaker than the target's. + minLength: 1, + maxLength: 99, + minimum: 0, + maximum: 1_000, + }, + }, + }, + }); + + const value = (parameters?.properties as Record>).value!; + // The intersection, per keyword direction: lower bounds take the max, upper bounds + // take the min. Both sides apply, so the surviving constraint is the stricter one. + expect(value.minLength).toBe(5); + expect(value.minimum).toBe(10); + expect(value.maxLength).toBe(10); + expect(value.maximum).toBe(100); + }); + + test("a tighter sibling assertion still wins", async () => { + // The other direction, so the fix cannot be "always prefer the target" - that would + // discard a genuine narrowing, which is the mirror-image bug. + const parameters = await emittedParameters("https://api.moonshot.ai/v1", { + name: "tightening_tool", + parameters: { + type: "object", + $defs: { Loose: { type: "string", minLength: 1, maxLength: 100 } }, + properties: { value: { $ref: "#/$defs/Loose", minLength: 5, maxLength: 10 } }, + }, + }); + + const value = (parameters?.properties as Record>).value!; + expect(value.minLength).toBe(5); + expect(value.maxLength).toBe(10); + }); + test("a deeply nested ref-free schema is bounded instead of exhausting the stack", async () => { // The second blocker: the expansion budget counts $ref inlines only, so a schema with // no refs at all walked unbounded. This nests far past any real tool. diff --git a/tests/provider-registry-parity.test.ts b/tests/provider-registry-parity.test.ts index 1f155d76df..53c7fc2076 100644 --- a/tests/provider-registry-parity.test.ts +++ b/tests/provider-registry-parity.test.ts @@ -369,8 +369,42 @@ describe("provider registry parity", () => { .filter(entry => entry.modelSuffixBracketStrip) .map(entry => entry.id); expect(zai?.modelContextWindows).toEqual({ "glm-5.3": 1_000_000, "glm-5.3[1m]": 1_000_000, "glm-5.3-flash": 1_000_000, "glm-5.2": 1_000_000, "glm-5.2[1m]": 1_000_000 }); - expect(zai?.modelDefaultReasoningEfforts).toEqual({ "glm-5.3": "max", "glm-5.3[1m]": "max" }); - expect(zai?.modelMaxOutputTokens).toEqual({ "glm-5.3": 131_072, "glm-5.3[1m]": 131_072 }); + // BUG-R5: glm-5.3-flash is a native VLM (docs.z.ai/guides/vlm/glm-5.3-flash), so it + // must never sit in noVisionModels - that list routes a model's images through the + // proxy's vision sidecar, which hands the model a text description of a picture it + // can read itself. The seeding pass classified it from the family name; the + // correction pass fixed the Alibaba entries and missed eight other providers. + // + // Asserted across the WHOLE registry rather than per provider, because the defect + // was not one entry being wrong - it was a set of entries drifting apart, and only + // a global assertion catches the next provider to seed it. + for (const entry of PROVIDER_REGISTRY) { + const flashIds = (entry.models ?? []).filter(id => String(id).includes("glm-5.3-flash")); + for (const id of flashIds) { + expect(entry.noVisionModels ?? []).not.toContain(id); + // An explicit modality declaration must include image. Absent is allowed: an + // unclassified model falls through to native passthrough, which is correct here. + const declared = entry.modelInputModalities?.[id]; + if (declared) expect(declared).toContain("image"); + } + } + // The sibling it is most often confused with stays text-only, so the assertion above + // cannot pass by making every GLM row a VLM. + expect(zai?.noVisionModels ?? []).toContain("glm-5.3"); + // `glm-5.3-flash` belongs in all three maps. It was seeded into the model list + // and the context map alone, so it advertised a 1M window with no effort ladder, + // no default effort and no output cap - and this assertion pinned that gap in + // place rather than catching it, because it was written from the incomplete + // state instead of from the family definition. + expect(zai?.modelDefaultReasoningEfforts).toEqual({ "glm-5.3": "max", "glm-5.3[1m]": "max", "glm-5.3-flash": "max" }); + expect(zai?.modelMaxOutputTokens).toEqual({ "glm-5.3": 131_072, "glm-5.3[1m]": 131_072, "glm-5.3-flash": 131_072 }); + // Every 5.3 row carries the same three-tier ladder. Asserted per member rather + // than as one object literal so adding a member cannot quietly skip it. + for (const id of ["glm-5.3", "glm-5.3[1m]", "glm-5.3-flash"]) { + expect(zai?.modelReasoningEfforts?.[id]).toEqual(["low", "high", "max"]); + expect(zai?.modelDefaultReasoningEfforts?.[id]).toBe("max"); + expect(zai?.modelMaxOutputTokens?.[id]).toBe(131_072); + } expect(providerConfigSeed(zai!).modelSuffixBracketStrip).toBe(true); expect(providerConfigSeed(zai!).modelDefaultReasoningEfforts?.["glm-5.3"]).toBe("max"); expect(deriveKeyLoginMap().zai.modelMaxOutputTokens?.["glm-5.3[1m]"]).toBe(131_072); From b0e893e71507777524d6bca314bb9952655e07cc Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 02:36:25 +0900 Subject: [PATCH 5/6] test(ci): import the cleanup helper without a lint suppression The hygiene gate flagged the new eslint-disable for no-require-imports, and it was right to: the repo already reaches CommonJS helpers through await import() (ci-workflows.test.ts:5030), so the suppression was covering for a spelling choice rather than an unavoidable constraint. The interop shim is deliberate rather than defensive. A .cjs module reached through ESM can arrive either directly or under default depending on the loader, and picking whichever object actually carries the planner keeps the test honest about what it is calling instead of asserting against undefined. --- tests/closed-pr-branch-cleanup.test.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/closed-pr-branch-cleanup.test.ts b/tests/closed-pr-branch-cleanup.test.ts index 63cd46f204..b18fdd7016 100644 --- a/tests/closed-pr-branch-cleanup.test.ts +++ b/tests/closed-pr-branch-cleanup.test.ts @@ -10,8 +10,12 @@ */ import { describe, expect, test } from "bun:test"; -// eslint-disable-next-line @typescript-eslint/no-require-imports -const cleanup = require("../.github/scripts/closed-pr-branch-cleanup.cjs") as { +/** + * Dynamic import rather than `require`: the repo's other CommonJS-helper tests reach for + * `await import(...)` (ci-workflows.test.ts:5030), and a `no-require-imports` suppression + * here would be a new lint suppression for a problem that has a supported spelling. + */ +interface CleanupModule { DEFAULT_GRACE_DAYS: number; KEEP_REASONS: Record; PROTECTED_BRANCHES: string[]; @@ -22,9 +26,15 @@ const cleanup = require("../.github/scripts/closed-pr-branch-cleanup.cjs") as { now?: number; graceDays?: number; }) => { deletions: { branch: string; pullRequests: number[] }[]; keeps: { branch: string; reason: string }[] }; -}; +} -const { KEEP_REASONS, planClosedPrBranchDeletions } = cleanup; +const cleanup = await import("../.github/scripts/closed-pr-branch-cleanup.cjs") as unknown as CleanupModule & { default?: CleanupModule }; +// A .cjs module reached through ESM interop may arrive under `default`; taking whichever +// carries the planner keeps the test honest about what it is calling. +const api: CleanupModule = typeof cleanup.planClosedPrBranchDeletions === "function" + ? cleanup + : cleanup.default!; +const { KEEP_REASONS, planClosedPrBranchDeletions } = api; const NOW = Date.parse("2026-08-27T00:00:00Z"); const LONG_AGO = new Date(NOW - 90 * 24 * 60 * 60 * 1000).toISOString(); From f25dab804716b9c251ad7a149362793f9533597c Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 02:52:17 +0900 Subject: [PATCH 6/6] test(providers): move two more assertions that pinned the incomplete state Both were written from what the registry happened to serve rather than from the family definition, so adding glm-5.3-flash to ZAI_GLM_53_MODELS broke them. codex-catalog expected modelSupportsReasoningSummaries to hold exactly the four older 5.3 ids. That map is derived from the family constant, and flash belongs in it: the reasoning-summary question and the vision-sidecar question have different answers for this model, and only the second one excludes it. cline-pass had my own editing mistake in it - I had added the id to a noVisionModels literal that is computed from CLINE_PASS_IMAGE_MODELS, so the expectation contradicted the derivation it was checking. Removed; the list already excludes flash on its own. --- tests/cline-pass-provider.test.ts | 1 - tests/codex-catalog.test.ts | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/cline-pass-provider.test.ts b/tests/cline-pass-provider.test.ts index 94072bc60a..c5e80cbecf 100644 --- a/tests/cline-pass-provider.test.ts +++ b/tests/cline-pass-provider.test.ts @@ -72,7 +72,6 @@ describe("ClinePass provider", () => { expect(entry?.modelMaxInputTokens).toBeUndefined(); expect(entry?.noVisionModels).toEqual([ "cline-pass/glm-5.3", - "cline-pass/glm-5.3-flash", "cline-pass/glm-5.2", "cline-pass/deepseek-v4-pro", "cline-pass/deepseek-v4-flash", diff --git a/tests/codex-catalog.test.ts b/tests/codex-catalog.test.ts index e507e8778f..a2f5b52fdc 100644 --- a/tests/codex-catalog.test.ts +++ b/tests/codex-catalog.test.ts @@ -4747,6 +4747,10 @@ describe("Codex catalog routed normalization", () => { "glm-5.2[1m]": true, "glm-5.3": true, "glm-5.3[1m]": true, + // glm-5.3-flash joined ZAI_GLM_53_MODELS, which is what modelSupportsReasoningSummaries + // is derived from. It belongs in the family for reasoning metadata even though it is + // excluded from the vision-sidecar list - the two answer different questions. + "glm-5.3-flash": true, }); });