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/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/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/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/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index b78717560e..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, }; @@ -261,6 +265,33 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise { 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/closed-pr-branch-cleanup.test.ts b/tests/closed-pr-branch-cleanup.test.ts new file mode 100644 index 0000000000..b18fdd7016 --- /dev/null +++ b/tests/closed-pr-branch-cleanup.test.ts @@ -0,0 +1,170 @@ +/** + * 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"; + +/** + * 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[]; + 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 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(); +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/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, }); }); 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 ef3ccda453..d1faf27699 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -12,6 +12,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { handleManagementAPI } from "../src/server/management-api"; import { LAYER_INVENTORY, readPromptLayers } from "../src/codex/prompt-layers"; +import type { ManagementPrincipal } from "../src/server/management-auth"; import type { OcxConfig } from "../src/types"; const MARKER = "# Auto-injected by opencodex"; @@ -69,6 +70,7 @@ async function call( pathname: string, fx: Fixture, body?: unknown, + principal: ManagementPrincipal | undefined = "gui-session", ): Promise<{ status: number; body: any; routed: boolean }> { 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; @@ -436,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 () => { @@ -565,6 +573,7 @@ describe("020 coverage completions", () => { store_unreadable: 409, invalid_characters: 400, write_superseded: 409, + write_failed: 500, recovery_required: 409, locked: 409, }); @@ -583,7 +592,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 +603,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 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/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 = { 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);