fix: stop deleting reused branches, and finish the glm-5.3-flash correction

.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<object>} input.pullRequests Pull requests with
- *   `headRefName`, `baseRefName`, `state`, `merged`, `closedAt`, and
- *   `isCrossRepository`.
- * @param {Array<string>} input.branches Branch names that currently exist.
+ *   `headRefName`, `headRefOid`, `baseRefName`, `state`, `merged`, `closedAt`,
+ *   and `isCrossRepository`.
+ * @param {Array<string|{name: string, oid?: string}>} 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<string, string|null>} */
+  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<string, object[]>} */
@@ -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

.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),
             );

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<string, "max" | "min"> = {
+  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<string, unknown>, 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;

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,31 +566,33 @@ 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*(?:#.*)?)$`);
   for (let i = 0; i < limit; i += 1) {
     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