fix(desktop): preserve ZIP member identity across bounded readers

Fixes #10172
Supersedes #10173

Co-Authored-By: gensokyo-dev <342580529+gensokyo-dev@users.noreply.github.com>

docs/desktop-application-export.md

@@ -599,10 +599,11 @@ when a DOCTYPE names a DTD. Known differences remain:
 
 - dconf databases are reported as binary, since REA reads them with `dconf`;
   inside an archive, one is `dconf` with an error, as in REA.
-- Zip entry names: a name in an Info-ZIP Unicode Path extra field is used only
-  where Python's `zipfile` reads it (3.12 and later), and an entry whose local
-  header names it differently from the central directory is damaged rather
-  than read.
+- Zip entries whose local header names them differently from the central
+  directory are damaged rather than read. Entry names follow REA's Unicode
+  Path selection: the final extra field replaces earlier claims, a bad version
+  or CRC falls back to the stored name, and the UTF-8 flag takes precedence.
+  Invalid UTF-8 in the selected name makes the archive view unreadable.
 - SQLite databases are read in place, read-only, rather than from a staged copy;
   a write lock the running app holds is waited out for a quarter of a second
   and then gives the file an `error`, and so does a table or column name that

src/domain/desktop/appExportPythonZip.ts

@@ -60,11 +60,8 @@ def _entry_bytes(archive, info):
     return data if len(data) == info.file_size else None
 
 
-def _zip_entry(archive, info, refusal):
+def _zip_entry(archive, info, refusal, name):
     """One entry of a zip view: its name, size and digest, and its own format and view, or why it was not read."""
-    # zipfile can override a UTF-8-flagged name with a Unicode Path extra field.
-    # REA's ZIP reader gives the explicitly UTF-8 stored name precedence.
-    name = info.orig_filename if info.flag_bits & 0x800 else info.filename
     entry = {"name": name, "size": info.file_size, "sha256": None, "format": None, "view": None,
              "view_error": refusal}
     if refusal is not None:
@@ -109,35 +106,48 @@ def _zip_end(file, size: int):
 
 
 def _zip_unicode_path(raw: bytes, flags: int, extra: bytes):
-    """An Info-ZIP Unicode Path when zip.js uses it: version 1, no UTF-8 flag, and the CRC-32 of the stored name."""
-    at = 0
+    """The last Info-ZIP Unicode Path, validated only after it replaces earlier claims, as in zip.js."""
+    at, data, length = 0, None, 0
+    while at + 4 <= len(extra):
+        kind, size = struct.unpack_from("<HH", extra, at)
+        if kind == 0x7075:
+            data, length = extra[at + 4:at + 4 + size], size
+        at += 4 + size
+    usable = (data is not None and length >= 5 and len(data) == length and data[0] == 1 and not flags & 0x800
+              and int.from_bytes(data[1:5], "little") == zlib.crc32(raw))
+    return data[5:] if usable else None
+
+
+def _zip_name(raw: bytes, flags: int, unicode_path):
+    """The exact REA name, shared by directory classification and the retained entry view."""
+    if flags & 0x800:
+        return raw.decode("utf-8")
+    return raw.decode("cp437") if unicode_path is None else unicode_path.decode("utf-8")
+
+
+def _zip_kept_record(fixed: bytes, rest: bytes, name_length: int, extra_length: int):
+    """Remove already-interpreted Unicode claims from the bounded decoder copy. zipfile otherwise interprets each
+    occurrence, retaining an earlier valid name or rejecting invalid UTF-8 that zip.js already superseded.
+    All other fields, comments and the original artifact bytes remain unchanged."""
+    raw, extra = rest[:name_length], rest[name_length:name_length + extra_length]
+    kept, at = bytearray(), 0
     while at + 4 <= len(extra):
         kind, length = struct.unpack_from("<HH", extra, at)
-        if kind == 0x7075:
-            data = extra[at + 4:at + 4 + length]
-            usable = (length >= 5 and len(data) == length and data[0] == 1 and not flags & 0x800
-                      and int.from_bytes(data[1:5], "little") == zlib.crc32(raw))
-            return data[5:] if usable else None
-        at += 4 + length
-    return None
-
-
-def _zip_ends_in_slash(raw: bytes, flags: int, extra: bytes):
-    """Whether an entry's name, as REA decodes it, ends in a slash; None when it must be UTF-8 and is not."""
-    utf8 = _zip_unicode_path(raw, flags, extra)
-    if utf8 is None and flags & 0x800:
-        utf8 = raw
-    if utf8 is None:
-        return raw.endswith(b"/")
-    try:
-        return utf8.decode("utf-8").endswith("/")
-    except UnicodeDecodeError:
-        return None
+        end = at + 4 + length
+        if end > len(extra):
+            raise zipfile.BadZipFile("Corrupt extra field")
+        if kind != 0x7075:
+            kept += extra[at:end]
+        at = end
+    kept += extra[at:]
+    header = bytearray(fixed)
+    struct.pack_into("<H", header, 30, len(kept))
+    return header + raw + kept + rest[name_length + extra_length:]
 
 
 def _zip_directory(file, size: int, keep: int):
     """An archive's central directory read a record at a time, keeping the first file entries' records, as many as keep allows, and
-    counting the rest, as REA reads it: (start, declared offset, records, count), or None when it cannot be read.
+    counting the rest, as REA reads it: (start, declared offset, records, count, names), or None when it cannot be read.
     Directories are told apart as zip.js tells them: a final slash, a Unix directory mode, or the MS-DOS attribute."""
     located = _zip_end(file, size)
     if located is None:
@@ -147,7 +157,7 @@ def _zip_directory(file, size: int, keep: int):
     if start < 0:
         return None
     file.seek(start)
-    records, count, at = [], 0, start
+    records, names, count, at = [], [], 0, start
     while at < end:
         fixed = file.read(46)
         if at + 46 > end or len(fixed) < 46 or fixed[:4] != ZIP_CENTRAL:
@@ -159,14 +169,14 @@ def _zip_directory(file, size: int, keep: int):
         at += 46 + name_length + extra_length + comment_length
         if len(rest) < name_length + extra_length + comment_length or at > end:
             return None
-        slash = _zip_ends_in_slash(rest[:name_length], flags, rest[name_length:name_length + extra_length])
-        if slash is None:
-            return None
-        if not (slash or (external >> 16) & 0o170000 == 0o040000 or (made_by >> 8 == 0 and external & 0x10)):
+        unicode_path = _zip_unicode_path(rest[:name_length], flags, rest[name_length:name_length + extra_length])
+        name = _zip_name(rest[:name_length], flags, unicode_path)
+        if not (name.endswith("/") or (external >> 16) & 0o170000 == 0o040000 or (made_by >> 8 == 0 and external & 0x10)):
             count += 1
             if len(records) < keep:
-                records.append(fixed + rest)
-    return start, declared, records, count
+                records.append(_zip_kept_record(fixed, rest, name_length, extra_length))
+                names.append(name)
+    return start, declared, records, count, names
 
 
 class _BoundedArchive(io.RawIOBase):
@@ -211,7 +221,7 @@ def _zip(file, size: int) -> dict:
         directory = _zip_directory(file, size, MAX_ZIP_ENTRIES)
         if directory is None:
             raise zipfile.BadZipFile("no readable central directory")
-        start, declared, records, count = directory
+        start, declared, records, count, names = directory
         kept = b"".join(records)
         offset = declared if declared <= 0xFFFFFFFF else start
         # zipfile follows a Zip64 locator signature wherever one sits before the end record, and the kept records
@@ -221,10 +231,10 @@ def _zip(file, size: int) -> dict:
                 + struct.pack("<4sIQI", ZIP64_LOCATOR, 0, offset + len(kept), 1)
                 + struct.pack("<4s4H2IH", ZIP_END, 0, 0, len(records), len(records), len(kept), offset, 0))
         with zipfile.ZipFile(_BoundedArchive(file, start, tail)) as archive:
-            for info in archive.infolist():
+            for info, name in zip(archive.infolist(), names, strict=True):
                 refusal = _zip_refusal(info, spent)
                 spent += info.file_size if refusal is None else 0
-                entries.append(_zip_entry(archive, info, refusal))
+                entries.append(_zip_entry(archive, info, refusal, name))
     except (zipfile.BadZipFile, zlib.error, EOFError, NotImplementedError, OSError, ValueError, struct.error):
         return {"format": "zip", "view": None, "error": "not a readable zip archive"}
     view = {"entries": entries, "entry_count": count, "truncated": count > len(entries)}

src/domain/desktop/zipArchive.fixture.ts

@@ -19,6 +19,21 @@ export interface ZipMember {
 
 const UTF8_NAME = 0x800;
 
+/** A version-1 Info-ZIP Unicode Path claim bound to the stored filename bytes. */
+export const unicodePathExtra = (
+  name: string,
+  path: string | Uint8Array,
+): Buffer => {
+  const text = Buffer.from(path);
+  const field = Buffer.alloc(9 + text.length);
+  field.writeUInt16LE(0x7075);
+  field.writeUInt16LE(field.length - 4, 2);
+  field[4] = 1;
+  field.writeUInt32LE(crc32(Buffer.from(name)), 5);
+  text.copy(field, 9);
+  return field;
+};
+
 /** A zip archive of the members in order, framed from the PKWARE APPNOTE records. */
 export const zipArchive = (members: readonly ZipMember[]): Buffer => {
   const locals: Buffer[] = [];

src/domain/desktop/zipDirectory.ts

@@ -84,19 +84,23 @@ const unicodePath = (
   view: DataView,
   { name, extra, comment, flags }: RecordName,
 ): Uint8Array | undefined => {
+  // zip.js indexes extra fields by ID with Map.set: the last occurrence
+  // replaces earlier ones, even when that last claim fails validation.
+  let selected: { at: number; length: number } | undefined;
   for (let at = extra; at + 4 <= comment;) {
     const length = view.getUint16(at + 2, true);
-    if (view.getUint16(at, true) === UNICODE_PATH)
-      return length >= 5 &&
-        at + 4 + length <= comment &&
-        bytes[at + 4] === 1 &&
-        (flags & UTF8_NAME) === 0 &&
-        view.getUint32(at + 5, true) === crc32(bytes.subarray(name, extra))
-        ? bytes.subarray(at + 9, at + 4 + length)
-        : undefined;
+    if (view.getUint16(at, true) === UNICODE_PATH) selected = { at, length };
     at += 4 + length;
   }
-  return undefined;
+  if (selected === undefined) return undefined;
+  const { at, length } = selected;
+  return length >= 5 &&
+    at + 4 + length <= comment &&
+    bytes[at + 4] === 1 &&
+    (flags & UTF8_NAME) === 0 &&
+    view.getUint32(at + 5, true) === crc32(bytes.subarray(name, extra))
+    ? bytes.subarray(at + 9, at + 4 + length)
+    : undefined;
 };
 
 /**

tests/boundary/cli/desktopZipUnicodeCli.test.ts

@@ -0,0 +1,205 @@
+import { createHash } from "node:crypto";
+import { readFile, writeFile } from "node:fs/promises";
+import { execFileSync } from "node:child_process";
+import { join } from "node:path";
+import { expect } from "vitest";
+import {
+  HOME,
+  capture,
+  change,
+  retained,
+  writeCaptureFolder,
+} from "../../../src/application/desktop/desktopCapture.fixture.js";
+import { desktopApplicationStateSchema } from "../../../src/domain/desktop/desktopState.js";
+import { parseEvidence } from "../../../src/domain/evidence.js";
+import {
+  unicodePathExtra,
+  zipArchive,
+} from "../../../src/domain/desktop/zipArchive.fixture.js";
+import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
+import { cliTest } from "../../support/cli/cliFixture.js";
+
+const retainedZipCapture = async () => {
+  const member = "settings.json";
+  const file = "Documents/state.zip";
+  const path = `${HOME}/${file}`;
+  const archives = [1, 2].map((zoom) =>
+    zipArchive([
+      {
+        name: member,
+        data: JSON.stringify({ zoom }),
+        ...(zoom === 1
+          ? {}
+          : {
+              centralExtra: Buffer.concat([
+                unicodePathExtra(member, "hidden/"),
+                unicodePathExtra(member, member),
+              ]),
+            }),
+      },
+    ]),
+  );
+  const [before, after] = archives;
+  const base = capture();
+  const [seed, end] = base.state.checkpoints;
+  if (
+    before === undefined ||
+    after === undefined ||
+    seed === undefined ||
+    end === undefined
+  )
+    throw new Error("missing fixture checkpoint");
+  const folder = await writeCaptureFolder(
+    {
+      ...base,
+      state: {
+        ...base.state,
+        checkpoints: [
+          { ...seed, retained: [...seed.retained, retained(path, before)] },
+          {
+            ...end,
+            changes: [...end.changes, change(path, { before, after })],
+            retained: [...end.retained, retained(path, after)],
+          },
+        ],
+      },
+    },
+    undefined,
+    archives,
+  );
+  return { member, file, path, archives, folder };
+};
+
+cliTest(
+  "retains a ZIP file through compiled state inspection and task export despite an earlier Unicode directory claim",
+  async ({ cli }) => {
+    const { member, file, path, archives, folder } = await retainedZipCapture();
+    const root = await createTestTempDirectory("desktop-zip-unicode-");
+    const absentTask = join(root, "absent.json");
+    await writeFile(
+      absentTask,
+      JSON.stringify({
+        name: "removed",
+        prompt: "Remove settings.json from the archive.",
+        checks: [
+          {
+            id: "removed",
+            kind: "value_absent",
+            file,
+            pointer: "/entries/[name=settings.json]",
+          },
+        ],
+      }),
+    );
+    const absent = await cli.run({
+      arguments: [
+        "export-desktop-application",
+        folder,
+        join(root, "absent-bundle"),
+        "--task",
+        absentTask,
+        "--json",
+      ],
+    });
+    // A complete-looking empty view once made this false removal task pass
+    // against the captured end, despite the file still being present.
+    expect(absent.exitCode, absent.stdout).toBe(1);
+    expect(absent.json).toMatchObject({ code: "invalid_request" });
+    const inspected = await cli.run({
+      arguments: [
+        "inspect-desktop-application-state",
+        folder,
+        "--state-path",
+        path,
+        "--json",
+      ],
+    });
+    expect(inspected.exitCode, inspected.stdout).toBe(0);
+    const state = desktopApplicationStateSchema.parse(
+      parseEvidence(inspected.json).normalized_result,
+    );
+    expect(state.paths[0]?.versions).toMatchObject(
+      [1, 2].map((zoom, i) => ({
+        sha256: createHash("sha256").update(archives[i]!).digest("hex"),
+        view: {
+          entries: [{ name: member, view: { zoom }, view_error: null }],
+          entry_count: 1,
+          truncated: false,
+        },
+      })),
+    );
+    const task = join(root, "task.json");
+    await writeFile(
+      task,
+      JSON.stringify({
+        name: "zoom",
+        prompt: "Set zoom to 2 and save the document.",
+        checks: [
+          {
+            id: "zoom",
+            kind: "value_equals",
+            file,
+            pointer: "/entries/[name=settings.json]/view/zoom",
+            value: 2,
+          },
+        ],
+      }),
+    );
+    const bundle = join(root, "bundle");
+    const exported = await cli.run({
+      arguments: [
+        "export-desktop-application",
+        folder,
+        bundle,
+        "--task",
+        task,
+        "--json",
+      ],
+    });
+    expect(exported.exitCode, exported.stdout).toBe(0);
+    expect(parseEvidence(exported.json).normalized_result).toMatchObject({
+      tasks: [{ name: "zoom", failing_on_seed: ["zoom"] }],
+    });
+    // Run the exact emitted reader, without the unavailable AgentEnv framework.
+    const module = await readFile(
+      join(bundle, "envs/notes/notes_app.py"),
+      "utf8",
+    );
+    const helpers = module
+      .split("@computer_app(")[0]!
+      .split("\n")
+      .filter((line) => !line.includes("from agentenv_protocol"))
+      .join("\n");
+    const verifier = (
+      await readFile(join(bundle, "artifacts/zoom-check/verify.py"), "utf8")
+    )
+      .split("\n")
+      .filter((line) => !line.includes("from agentenv_protocol"))
+      .join("\n");
+    const copies = [join(root, "seed.zip"), join(root, "end.zip")];
+    await Promise.all(copies.map((copy, i) => writeFile(copy, archives[i]!)));
+    const script = join(root, "reader.py");
+    await writeFile(
+      script,
+      `${helpers}\n${verifier}\nseen = [_view(Path(p)) for p in json.load(sys.stdin)]\nprint(json.dumps({"views": [v["view"] for v in seen], "grades": [[r["result"] for r in evaluate(CHECKS, {${JSON.stringify(file)}: v})] for v in seen]}))\n`,
+    );
+    const views: unknown = JSON.parse(
+      execFileSync(
+        process.platform === "win32" ? "python" : "python3",
+        ["-I", "-B", script],
+        { input: JSON.stringify(copies), encoding: "utf8" },
+      ),
+    );
+    expect(views).toMatchObject({
+      views: [1, 2].map((zoom) => ({
+        entries: [{ name: member, view: { zoom } }],
+        entry_count: 1,
+        truncated: false,
+      })),
+      grades: [[false], [true]],
+    });
+    expect(await Promise.all(copies.map((copy) => readFile(copy)))).toEqual(
+      archives,
+    );
+  },
+);

tests/boundary/providers/desktopZipPython.test.ts

@@ -1,16 +1,23 @@
 import { readFile } from "node:fs/promises";
 import { join } from "node:path";
 import { crc32 } from "node:zlib";
+import { Uint8ArrayReader, ZipReader } from "@zip.js/zip.js";
 
 import { describe, expect, it } from "vitest";
 
 import { readZipView } from "../../../src/application/desktop/DesktopZipView.js";
+import {
+  decodeZipEntryText,
+  ZIP_READER_OPTIONS,
+  zipEntryPath,
+} from "../../../src/artifacts/ZipArtifactReader.js";
 import type { DesktopTaskCheck } from "../../../src/domain/desktop/appTask.js";
 import type { JsonValue } from "../../../src/domain/jsonValue.js";
 import { detectStateFormat } from "../../../src/domain/desktop/stateFormats.js";
 import {
   emptyEntriesArchive,
   odtArchive,
+  unicodePathExtra,
   withZip64End,
   zipArchive,
   type ZipMember,
@@ -66,13 +73,7 @@ const sqliteHeader = Buffer.concat([
 
 it("gives the UTF-8 flag precedence over a Unicode Path for names and formats", async () => {
   const storedName = "legacy.txt";
-  const unicodeName = Buffer.from("café.json");
-  const extra = Buffer.alloc(9 + unicodeName.length);
-  extra.writeUInt16LE(0x7075);
-  extra.writeUInt16LE(extra.length - 4, 2);
-  extra[4] = 1;
-  extra.writeUInt32LE(crc32(Buffer.from(storedName)), 5);
-  unicodeName.copy(extra, 9);
+  const extra = unicodePathExtra(storedName, "café.json");
   const archive = zipArchive([
     { name: storedName, data: "42", flags: 0x800, centralExtra: extra },
     { name: "other.txt", data: "42", centralExtra: extra },
@@ -86,6 +87,123 @@ it("gives the UTF-8 flag precedence over a Unicode Path for names and formats",
   });
 });
 
+it.each([
+  "file",
+  "crc",
+  "version",
+  "short",
+  "superseded-utf8",
+  "flagged",
+  "empty",
+  "nul",
+])(
+  "keeps the ZIP decoder's final Unicode Path choice through bounded views (%s)",
+  async (kind) => {
+    const name = "settings.json";
+    const displayed =
+      kind === "empty" ? "" : kind === "nul" ? "kept\0.json" : name;
+    const first = unicodePathExtra(
+      name,
+      kind === "superseded-utf8" ? Buffer.from([0xff]) : "hidden/",
+    );
+    let last = unicodePathExtra(name, displayed);
+    if (kind === "crc") last.writeUInt32LE(0, 5);
+    if (kind === "version") last[4] = 2;
+    if (kind === "short") last = Buffer.from([0x75, 0x70, 1, 0, 1]);
+    const archive = zipArchive([
+      {
+        name,
+        data: '{"zoom":2}',
+        flags: kind === "flagged" ? 0x800 : 0,
+        centralExtra: Buffer.concat([first, last]),
+      },
+      { name: "control.txt", data: "unaffected" },
+    ]);
+    // The complete production decoder is the semantic control for the bounded
+    // preflight, including invalid final claims falling back to the raw name.
+    const reader = new ZipReader(
+      new Uint8ArrayReader(archive),
+      ZIP_READER_OPTIONS,
+    );
+    try {
+      expect(
+        (await reader.getEntries({ decodeText: decodeZipEntryText })).map(
+          (entry) => ({
+            name: zipEntryPath(entry),
+            directory: entry.directory,
+          }),
+        ),
+      ).toEqual([
+        { name: displayed, directory: false },
+        { name: "control.txt", directory: false },
+      ]);
+    } finally {
+      await reader.close();
+    }
+    expect(await sameView(archive)).toMatchObject({
+      entries: [
+        {
+          name: displayed,
+          format: "json",
+          view: { zoom: 2 },
+          view_error: null,
+        },
+        { name: "control.txt", view_error: null },
+      ],
+      entry_count: 2,
+      truncated: false,
+    });
+  },
+);
+
+it("refuses invalid UTF-8 in the selected Unicode claim while preserving an unrelated extra field", async () => {
+  const name = "settings.json";
+  const opaque = Buffer.from([0xfe, 0xca, 3, 0, 1, 2, 3]);
+  const archive = (last: Buffer) =>
+    zipArchive([
+      {
+        name,
+        data: '{"zoom":2}',
+        centralExtra: Buffer.concat([
+          unicodePathExtra(name, "hidden/"),
+          opaque,
+          last,
+        ]),
+      },
+    ]);
+  expect(await sameView(archive(unicodePathExtra(name, name)))).toMatchObject({
+    entry_count: 1,
+    entries: [{ name, view: { zoom: 2 } }],
+    truncated: false,
+  });
+  const invalid = archive(unicodePathExtra(name, Buffer.from([0xff])));
+  await expectSameViews([invalid]);
+  expect(await readZipView(invalid)).toEqual({
+    ok: false,
+    error: "not a readable zip archive",
+  });
+});
+
+it("excludes a final Unicode directory without consuming a retained file slot", async () => {
+  const name = "settings.json";
+  const archive = zipArchive([
+    {
+      name,
+      data: "not a file view",
+      centralExtra: Buffer.concat([
+        unicodePathExtra(name, name),
+        unicodePathExtra(name, "hidden/"),
+      ]),
+    },
+    { name: "control.json", data: '{"kept":true}' },
+  ]);
+  expect(await sameView(archive)).toMatchObject({
+    entries: [{ name: "control.json", view: { kept: true } }],
+    entry_count: 1,
+    truncated: false,
+  });
+});
+
 describe("zip views in the exported module", () => {
   it("lists entries in archive order with the formats and views REA reads", async () => {
     const members: ZipMember[] = [