fix: address all PR review findings from bots

- HIGH: Fix multi-step patch chain corruption by alternating between
  two intermediate files (A/B) instead of reading and writing the same
  file. Prevents mmap invalidation for 3+ step chains.

- MEDIUM: Use versioned nightly tag (nightly-<version>) instead of
  rolling :nightly tag for threshold calculation in resolveNightlyDelta.
  Ensures the 60% threshold reflects the target version's binary size.

- LOW: Add missing 'await' on applyPatchChain return in
  resolveNightlyDelta to preserve stack traces on rejection.

- LOW: Remove redundant chmodSync from applyPatchChain — the caller
  (downloadBinaryToTemp) already sets 0o755 for both delta and full
  download paths.

- CodeQL: Replace includes('api.github.com') with
  startsWith('https://api.github.com/') in test mocks to prevent
  incomplete URL substring sanitization.

Author: BYK

src/lib/delta-upgrade.ts

@@ -17,7 +17,8 @@
  * - Any error occurs during patch download or application
  */
 
-import { chmodSync } from "node:fs";
+import { unlinkSync } from "node:fs";
+
 import { applyPatch } from "./bspatch.js";
 import { CLI_VERSION } from "./constants.js";
 import {
@@ -621,9 +622,12 @@ export async function resolveNightlyDelta(
 ): Promise<string | null> {
   const token = await getAnonymousToken();
 
-  // Get the .gz layer size from the nightly manifest for threshold
+  // Get the .gz layer size from the target version's manifest for threshold.
+  // Use the versioned tag (nightly-<version>) so the threshold reflects the
+  // actual binary being upgraded to, not the latest rolling nightly.
   const binaryName = getPlatformBinaryName();
-  const nightlyManifest = await fetchManifest(token, "nightly");
+  const targetTag = `nightly-${targetVersion}`;
+  const nightlyManifest = await fetchManifest(token, targetTag);
   const gzLayer = nightlyManifest.layers.find((l) => {
     const title = l.annotations?.["org.opencontainers.image.title"];
     return title === `${binaryName}.gz`;
@@ -642,15 +646,31 @@ export async function resolveNightlyDelta(
     return null;
   }
 
-  return applyPatchChain(chain, oldBinaryPath, destPath);
+  return await applyPatchChain(chain, oldBinaryPath, destPath);
+}
+
+/** Remove intermediate patching files, ignoring errors. */
+function cleanupIntermediates(destPath: string): void {
+  for (const suffix of [".patching.a", ".patching.b"]) {
+    try {
+      unlinkSync(`${destPath}${suffix}`);
+    } catch {
+      // Ignore cleanup errors
+    }
+  }
 }
 
 /**
  * Apply a resolved patch chain sequentially and verify the result.
  *
  * For single-patch chains, applies directly from old binary to dest.
- * For multi-patch chains, uses an intermediate temp file: each step
- * produces a new file that becomes the input for the next step.
+ * For multi-patch chains, alternates between two intermediate files
+ * so that read (mmap) and write never target the same path — writing
+ * to the mmap source would truncate it and corrupt the output.
+ *
+ * Does **not** set executable permissions — the caller
+ * (`downloadBinaryToTemp`) handles that uniformly for both delta
+ * and full-download paths.
  *
  * @param chain - Resolved patch chain with patches and expected hash
  * @param oldBinaryPath - Path to the original binary
@@ -666,33 +686,30 @@ export async function applyPatchChain(
   let currentOldPath = oldBinaryPath;
   let sha256 = "";
 
-  // For multi-step chains, intermediate results go to destPath with a step suffix
-  const intermediatePath = `${destPath}.patching`;
+  // Alternate between two intermediate paths to avoid reading and writing
+  // the same file (mmap'd read + writer truncation = corruption).
+  const intermediateA = `${destPath}.patching.a`;
+  const intermediateB = `${destPath}.patching.b`;
 
   for (let i = 0; i < chain.patches.length; i++) {
     const patch = chain.patches[i];
     if (!patch) {
       throw new Error(`Missing patch at index ${i}`);
     }
     const isLast = i === chain.patches.length - 1;
-    const outputPath = isLast ? destPath : intermediatePath;
+    const intermediate = i % 2 === 0 ? intermediateA : intermediateB;
+    const outputPath = isLast ? destPath : intermediate;
 
     sha256 = await applyPatch(currentOldPath, patch.data, outputPath);
 
-    // For multi-step: the output of this step becomes the input for the next
     if (!isLast) {
-      currentOldPath = intermediatePath;
+      currentOldPath = outputPath;
     }
   }
 
-  // Clean up intermediate file if it exists
+  // Clean up intermediate files
   if (chain.patches.length > 1) {
-    try {
-      const { unlinkSync: unlink } = await import("node:fs");
-      unlink(intermediatePath);
-    } catch {
-      // Ignore cleanup errors
-    }
+    cleanupIntermediates(destPath);
   }
 
   // Verify the final SHA-256 matches
@@ -702,10 +719,5 @@ export async function applyPatchChain(
     );
   }
 
-  // Set executable permission (Unix only)
-  if (process.platform !== "win32") {
-    chmodSync(destPath, 0o755);
-  }
-
   return sha256;
 }

test/isolated/delta-upgrade.test.ts

@@ -133,7 +133,7 @@ describe("resolveStableDelta", () => {
 
     mockFetch(async (url) => {
       const urlStr = String(url);
-      if (urlStr.includes("api.github.com")) {
+      if (urlStr.startsWith("https://api.github.com/")) {
         return new Response(JSON.stringify(releases), { status: 200 });
       }
       if (urlStr === patchUrl) {
@@ -198,7 +198,7 @@ describe("resolveNightlyDelta", () => {
           status: 200,
         });
       }
-      if (urlStr.includes("/manifests/nightly")) {
+      if (urlStr.includes("/manifests/nightly-")) {
         return new Response(
           JSON.stringify({
             schemaVersion: 2,
@@ -250,7 +250,7 @@ describe("resolveNightlyDelta", () => {
           status: 200,
         });
       }
-      if (urlStr.includes("/manifests/nightly")) {
+      if (urlStr.includes("/manifests/nightly-")) {
         return new Response(
           JSON.stringify({
             schemaVersion: 2,
@@ -372,7 +372,7 @@ describe("attemptDeltaUpgrade", () => {
     // Return garbage patch data
     mockFetch(async (url) => {
       const urlStr = String(url);
-      if (urlStr.includes("api.github.com")) {
+      if (urlStr.startsWith("https://api.github.com/")) {
         return new Response(JSON.stringify(releases), { status: 200 });
       }
       if (urlStr === patchUrl) {

test/lib/delta-upgrade.test.ts

@@ -915,7 +915,7 @@ describe("resolveStableChain", () => {
   ): void {
     mockFetch(async (url) => {
       const urlStr = String(url);
-      if (urlStr.includes("api.github.com")) {
+      if (urlStr.startsWith("https://api.github.com/")) {
         return new Response(JSON.stringify(releases), { status: 200 });
       }
       const patchData = patches.get(urlStr);
@@ -1371,7 +1371,8 @@ describe("applyPatchChain", () => {
     // the cleanup logic works by checking intermediate files don't leak.
     const oldPath = join(fixturesDir, "small-old.bin");
     const destPath = tempFile("multi-chain-out.bin");
-    const intermediatePath = `${destPath}.patching`;
+    const intermediateA = `${destPath}.patching.a`;
+    const intermediateB = `${destPath}.patching.b`;
     const patchData = await Bun.file(
       join(fixturesDir, "small.trdiff10")
     ).bytes();
@@ -1405,19 +1406,17 @@ describe("applyPatchChain", () => {
       if (existsSync(destPath)) {
         unlinkSync(destPath);
       }
-      if (existsSync(intermediatePath)) {
-        unlinkSync(intermediatePath);
+      for (const p of [intermediateA, intermediateB]) {
+        if (existsSync(p)) {
+          unlinkSync(p);
+        }
       }
     }
   });
 
-  test("sets executable permission on output (Unix)", async () => {
-    if (process.platform === "win32") {
-      return; // Skip on Windows
-    }
-
+  test("creates output file that is readable", async () => {
     const oldPath = join(fixturesDir, "small-old.bin");
-    const destPath = tempFile("perms-out.bin");
+    const destPath = tempFile("output-readable.bin");
     const patchData = await Bun.file(
       join(fixturesDir, "small.trdiff10")
     ).bytes();
@@ -1443,7 +1442,6 @@ describe("applyPatchChain", () => {
       await applyPatchChain(chain, oldPath, destPath);
 
       const stat = Bun.file(destPath);
-      // File should exist and be readable
       expect(await stat.exists()).toBe(true);
     } finally {
       if (existsSync(destPath)) {