Merge pull request #612: fix(spreadsheet-preview): open openpyxl workbooks that have a comment, table or chart

.changeset/xlsx-openpyxl-preview.md

@@ -0,0 +1,5 @@
+---
+'aicodeman': patch
+---
+
+Spreadsheet preview: workbooks written by openpyxl (the Python library most agents use to make .xlsx files) now open when they contain a cell comment, a table or a chart. Before, any one of those made the whole preview fail with "Spreadsheet preview failed".

docs/architecture-invariants.md

@@ -381,6 +381,8 @@ A file path an agent prints is a link on both surfaces it can appear on, and cli
 
 **XLSX previews parse in the browser worker, and ExcelJS only ever sees what admission checked.** An `.xlsx` joins the raw routes like any other file (`?preview=true` caps it at 10 MB with a 413); the server never parses it. `spreadsheet-preview.js` hands the bytes to `spreadsheet-preview-worker.js`, the ONLY place the pinned `exceljs`/`fflate` vendor bundles load: fflate and `spreadsheet-xlsx-core.js` at worker start, ExcelJS only after `admitXlsx()` passes, and the page itself never loads either. `admitXlsx()` streams every entry through fflate and enforces the entry count, per-entry and total inflated bytes (64 MB), compression ratio, worksheet, cell, merge and style caps on the actual inflated output, refusing (never truncating) a workbook that trips one. ⚠️ Admission walks LOCAL headers while ExcelJS (JSZip) reads the CENTRAL directory, so overlapping entries (a stored entry hiding a whole `sheet1.xml`, a one-cell decoy later in the stream) once let a file be admitted as 1 cell and parsed as 300k. The worker therefore never hands ExcelJS the fetched bytes: it gets `buildAdmittedArchive()`, a STORE-only `fflate.zipSync(entries, { level: 0 })` of exactly the entries admission inflated, and a name streamed twice is refused. That costs one transient copy of the inflated entries (bounded by the same 64 MB cap) and is pinned by the overlapping-entry fixture in `test/spreadsheet-preview-worker.test.ts`. ⚠️ Admission also has to bound what ExcelJS EXPANDS, not just what it reads: ExcelJS 4.4.0 builds one object per covered cell of a `<mergeCell>`, per address of a `<dataValidation sqref>` and per index up to `<col max>`, so a 6.5 KB file admitted as one cell once cost a gigabyte. The counter therefore charges each merge its full AREA against the cell caps (an unparseable `ref` is refused), refuses a `<col>` whose `min`/`max` is past 16384, and the worker loads with `ignoreNodes: ['dataValidations']` (the preview never shows validations). Defined names expand the same way and live in `xl/workbook.xml`, where admission reads only the `<sheet>` ids: ExcelJS's `DefinedNames` model setter builds one object per cell of every range (a whole-sheet name exhausted a 4 GB heap), so right after `new ExcelJS.Workbook()` the worker replaces `_definedNames.model` with an `Object.defineProperty` stub whose getter returns `[]` and whose setter drops the value. Print areas and titles are split off in the workbook xform's reconcile, before that setter runs, so they are unaffected, and `defineProperty` throws if a future ExcelJS renames `_definedNames` rather than silently expanding again. ⚠️ Every name admission uses is the one ExcelJS will SEE, not the stored one: JSZip resolves each entry name on load (`utils.resolve`: `.` and empty middle segments dropped, `..` pops a segment) and ExcelJS then strips one leading `/` and matches worksheets with an UNANCHORED `xl/worksheets/sheet<N>.xml`. Checking the stored name once let `/xl/worksheets/sheet1.xml`, `xl/./...`, `xl//...`, `xl/xl/worksheets/sheet1.xml` and `xl/worksheets/sheet1.xml.x` skip every counter. `excelJsEntryName()` mirrors both steps; admission refuses two entries that resolve to one name, keys `inflatedEntries` (so the rebuilt archive) on the resolved name, picks the worksheet and styles counters from it, and counts any name matching ExcelJS's unanchored worksheet pattern as a worksheet. The local-versus-central consistency checks still compare the stored names. The counter reads `<mergeCell>`/`<col>` attributes IN ORDER from the tag name with a sticky regex that consumes each quoted value whole (a raw `>` or the other quote character is legal inside a value, so a first-match search could be fed a fake `ref`/`max`), and refuses a tag whose attributes do not parse up to `>` or repeat a name. It counts every `<row>` (ExcelJS keeps one Row object per element, cells or not) against per-sheet and total row caps, and reads each `<row>`'s attributes the same way. ⚠️ The INDEX a row or sheet claims is a cost too, not just how many there are: ExcelJS stores a row at `_rows[r - 1]` and walks `_rows` up to the largest index on every `eachRow` and `sheet.model` (five rows plus one `<row r="50000000">` took 5 s to load and 1.7 s per tile), and stores a sheet at `_worksheets[sheetId]`, which its `worksheets` getter slices and sorts (`sheetId="30000000"` on a one-cell workbook took 1.6 s and 557 MB). So a `<row r>` that is not plain digits in 1 to 1,048,576 is refused (an absent `r` is fine), and a counter for the resolved `xl/workbook.xml` reads every `<sheet>` tag in order and refuses one that does not parse or whose `sheetId` is not plain digits up to `LIMITS.maxSheetId` (65535; real ids are small, and the `(?=[\s/>])` lookahead keeps the `<sheets>` container out). The counter also counts every `<xf>` in styles.xml with no list-tracking state (a `</cellXfs>` inside a comment used to desync it). It carries everything from the last `<` into the next inflate chunk,Line truncated
 
+⚠️ **ExcelJS follows a worksheet's comment, comment-note (VML), table and drawing links UNCHECKED, so the worker repairs its parsed model before it links it.** ExcelJS 4.4.0's `reconcile` looks each relationship `Target` up verbatim in maps keyed by its own spelling (`../comments1.xml`, `../drawings/vmlDrawing1.vml`, `../tables/table1.xml`, `drawing1`) and dereferences the result. openpyxl, which most agents write spreadsheets with, uses absolute Targets, its own part names (`/xl/comments/comment1.xml`, which ExcelJS never even loads) and a drawing XML ExcelJS parses to nothing, so a workbook with ONE comment, table or chart threw (`reading 'comments'`, `'name'`, `'anchors'`) and the whole preview failed. Right after `new ExcelJS.Workbook()` the worker wraps `workbook.xlsx.reconcile` (which `load()` calls once on the parsed model) to run `repairExcelJsModel()` (core) first: it re-points a link to the key ExcelJS stored for the same part (openpyxl's `/xl/tables/table1.xml` becomes `../tables/table1.xml`, so the table is kept), and drops a link, `<tablePart>`, `<drawing>` or `<hyperlink>` reference, or parsed-to-nothing drawing/table that would dereference nothing. It never adds anything, looks keys up as own properties only (a `__proto__` Target resolves to nothing), and the preview shows none of these parts. The wrap throws if a future ExcelJS renames `reconcile`. Real openpyxl 3.1.5 fixtures (`test/fixtures/xlsx/openpyxl-{comment,table,chart}.xlsx`) fail the worker tests without it.
+
 ### Filesystem path picker
 
 **Filesystem path picker** (Link Existing "Browse" button + the extended mobile keyboard's `📁 Path` key): a lazy one-directory-at-a-time browser over `GET /api/filesystem/browse`, with `GET /api/filesystem/preview` serving the tapped file. It starts at the active session's working directory (falling back to the Codeman Cases root, then `/mnt/d`, then the first root), hides dot entries, and inserts the chosen path **without** Enter so the prompt is not submitted. The companion `⌫ All` key clears only the current unsent prompt buffer and must never emit the agent's `/clear` command.

src/web/public/spreadsheet-preview-worker.js

@@ -171,6 +171,17 @@ async function loadWorkbook(bytes) {
   // defineProperty throws if a future ExcelJS renames `_definedNames`, rather
   // than silently expanding again.
   Object.defineProperty(nextWorkbook._definedNames, 'model', { configurable: true, get: () => [], set: () => {} });
+  // ExcelJS follows a worksheet's comment, VML, table and drawing links
+  // unchecked, and openpyxl spells them so the lookup finds nothing: one comment,
+  // table or chart failed the whole preview. `load()` calls `this.reconcile()`
+  // once on the parsed model; repair the model first (core.repairExcelJsModel).
+  // Throws if a future ExcelJS renames reconcile, rather than failing again.
+  const reconcile = nextWorkbook.xlsx.reconcile;
+  if (typeof reconcile !== 'function') throw new Error('ExcelJS reconcile is unavailable');
+  nextWorkbook.xlsx.reconcile = function reconcileRepaired(model, options) {
+    core.repairExcelJsModel(model);
+    return reconcile.call(this, model, options);
+  };
   // ExcelJS expands every address of a `<dataValidation sqref>` into its own
   // object (a whole-column dropdown is a million), and the preview never shows
   // validations, so they are not parsed at all. `maxRows` is a per-sheet

src/web/public/spreadsheet-preview.js

@@ -20,7 +20,7 @@
 (function initSpreadsheetPreview(global) {
   'use strict';
 
-  const SPREADSHEET_ASSET_VERSION = '911680fac09d';
+  const SPREADSHEET_ASSET_VERSION = '0833a414e785';
   const MAX_PREVIEW_BYTES = 10 * 1024 * 1024;
   const DEFAULT_TIMEOUT_MS = 20000;
   const MAX_SCROLL_PX = 8000000;

src/web/public/spreadsheet-xlsx-core.js

@@ -472,6 +472,102 @@
     return zipApi.zipSync(admission.entries, { level: 0 });
   }
 
+  // ExcelJS (4.4.0, lib/xlsx/xlsx.js + xform/sheet/worksheet-xform.js `reconcile`)
+  // links a worksheet to its comments, comment notes (VML), tables and drawing by
+  // looking the relationship Target up VERBATIM in maps keyed by its OWN spelling
+  // (`../comments1.xml`, `../drawings/vmlDrawing1.vml`, `../tables/table1.xml`,
+  // `drawing1`), then dereferences the result unchecked. openpyxl, which most
+  // agents write spreadsheets with, uses absolute Targets, its own part names
+  // (`/xl/comments/comment1.xml`) and a drawing XML ExcelJS parses to nothing, so
+  // a workbook with ONE comment, table or chart threw ("reading 'comments'",
+  // "'name'", "'anchors'") before a single cell was shown.
+  //
+  // This runs on ExcelJS's parsed model right before that reconcile. It only
+  // re-points a link to the key ExcelJS itself stored for the same part, or drops
+  // a link (or a drawing) that would dereference nothing; nothing is added. The
+  // preview shows none of these parts. Lookups are own-property only, so a Target
+  // or rId such as `__proto__` cannot resolve through Object.prototype.
+  const REL_TYPE = 'http://schemas.openxmlformats.org/officeDocument/2006/relationships/';
+  const EXCELJS_DRAWING_TARGET = /\/drawings\/([a-zA-Z0-9]+)[.][a-zA-Z]{3,4}$/;
+
+  function ownValue(map, key) {
+    return map && typeof key === 'string' && Object.prototype.hasOwnProperty.call(map, key) ? map[key] : undefined;
+  }
+
+  // The key ExcelJS gives a part it loaded from `xl/<path>` is `../<path>`
+  // (relative to xl/worksheets/). A Target that names the same part another way
+  // (`/xl/tables/table1.xml`) is rewritten to that key.
+  function worksheetRelativeTarget(target) {
+    const path = excelJsEntryName(target[0] === '/' ? target : `xl/worksheets/${target}`);
+    return path.startsWith('xl/') ? `../${path.slice(3)}` : null;
+  }
+
+  function resolveTarget(rel, map, usable) {
+    if (typeof rel.Target !== 'string') return false;
+    if (usable(ownValue(map, rel.Target))) return true;
+    const key = worksheetRelativeTarget(rel.Target);
+    if (key && usable(ownValue(map, key))) {
+      rel.Target = key;
+      return true;
+    }
+    return false;
+  }
+
+  function repairExcelJsModel(model) {
+    if (!model || typeof model !== 'object') return model;
+    const isObject = (value) => Boolean(value) && typeof value === 'object';
+    const drawings = isObject(model.drawings) ? model.drawings : {};
+    // ExcelJS reconciles every drawing that has rels and reads `.anchors` off it.
+    for (const name of Object.keys(drawings)) {
+      if (!isObject(drawings[name]) || !Array.isArray(drawings[name].anchors)) {
+        delete drawings[name];
+        if (isObject(model.drawingRels)) delete model.drawingRels[name];
+      }
+    }
+    // ExcelJS also reconciles every table it parsed, so one it parsed to nothing goes.
+    if (isObject(model.tables)) {
+      for (const key of Object.keys(model.tables)) if (!isObject(model.tables[key])) delete model.tables[key];
+    }
+    const worksheetRels = isObject(model.worksheetRels) ? model.worksheetRels : {};
+    for (const worksheet of Array.isArray(model.worksheets) ? model.worksheets : []) {
+      if (!isObject(worksheet)) continue;
+      const list = ownValue(worksheetRels, String(worksheet.sheetNo));
+      const rels = Array.isArray(list) ? list : [];
+      const kept = rels.filter((rel) => {
+        if (!isObject(rel)) return false;
+        if (rel.Type === `${REL_TYPE}comments`) return resolveTarget(rel, model.comments, isObject);
+        if (rel.Type === `${REL_TYPE}vmlDrawing`) {
+          return resolveTarget(rel, model.vmlDrawings, (vml) => isObject(vml) && Array.isArray(vml.comments));
+        }
+        if (rel.Type === `${REL_TYPE}table`) return resolveTarget(rel, model.tables, isObject);
+        return true;
+      });
+      if (Array.isArray(list)) worksheetRels[String(worksheet.sheetNo)] = kept;
+      const byId = new Map(kept.map((rel) => [rel.Id, rel]));
+      // `<tablePart r:id>`, `<drawing r:id>` and `<hyperlink r:id>` are followed
+      // through the rels unchecked, so each must name a rel that is still there.
+      if (Array.isArray(worksheet.tables)) {
+        worksheet.tables = worksheet.tables.filter((part) => {
+          const rel = isObject(part) ? byId.get(part.rId) : undefined;
+          return Boolean(rel) && rel.Type === `${REL_TYPE}table`;
+        });
+      }
+      if (isObject(worksheet.drawing)) {
+        const rel = byId.get(worksheet.drawing.rId);
+        const match = rel && typeof rel.Target === 'string' ? rel.Target.match(EXCELJS_DRAWING_TARGET) : null;
+        if (!rel || typeof rel.Target !== 'string' || (match && !ownValue(drawings, match[1]))) {
+          worksheet.drawing = undefined;
+        }
+      }
+      if (Array.isArray(worksheet.hyperlinks)) {
+        worksheet.hyperlinks = worksheet.hyperlinks.filter(
+          (link) => isObject(link) && (!link.rId || byId.has(link.rId))
+        );
+      }
+    }
+    return model;
+  }
+
   function parseCellRef(ref) {
     const match = /^\$?([A-Z]{1,3})\$?([1-9]\d*)$/i.exec(String(ref || ''));
     if (!match) return null;
@@ -983,6 +1079,7 @@
     createXmlCounter,
     admitXlsx,
     buildAdmittedArchive,
+    repairExcelJsModel,
     excelJsEntryName,
     parseCellRef,
     parseRange,

test/spreadsheet-preview-worker.test.ts

@@ -1070,3 +1070,41 @@ describe('spreadsheet preview worker: element budget', () => {
     expect(harness.imports.some((url) => url.includes('exceljs'))).toBe(false);
   }, 60_000);
 });
+
+// Real files written by openpyxl 3.1.5, the library most agents write
+// spreadsheets with: a "Data" sheet (month, revenue x6) plus ONE feature each.
+// ExcelJS threw on all three before repairExcelJsModel, so the preview failed
+// on the whole workbook. Regenerate with openpyxl's Comment, Table and BarChart.
+function openpyxlFixture(name: string): ArrayBuffer {
+  return toArrayBuffer(new Uint8Array(readFileSync(resolve(root, 'test/fixtures/xlsx', name))));
+}
+
+describe('spreadsheet preview worker: workbooks written by openpyxl', () => {
+  for (const [file, feature] of [
+    ['openpyxl-comment.xlsx', 'a cell comment'],
+    ['openpyxl-table.xlsx', 'a table'],
+    ['openpyxl-chart.xlsx', 'a bar chart'],
+  ] as const) {
+    it(`previews a sheet that has ${feature}`, async () => {
+      const harness = createHarness();
+      const metadata = await loadMetadata(harness, openpyxlFixture(file));
+      expect(metadata.sheets.map((sheet: { name: string }) => sheet.name)).toEqual(['Data']);
+      expect(metadata.sheets[0]).toMatchObject({ rows: 7, cols: 2 });
+      const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 1, r2: 7, c2: 2 });
+      expect(tile.type).toBe('tile');
+      expect(tile.cells).toEqual(
+        expect.arrayContaining([
+          expect.objectContaining({ row: 1, col: 1, text: 'month' }),
+          expect.objectContaining({ row: 7, col: 1, text: '2026-06' }),
+          expect.objectContaining({ row: 7, col: 2, text: '600' }),
+        ])
+      );
+    });
+  }
+
+  it('still flags the chart it cannot show', async () => {
+    const harness = createHarness();
+    const metadata = await loadMetadata(harness, openpyxlFixture('openpyxl-chart.xlsx'));
+    expect(metadata.warnings).toEqual(expect.arrayContaining(['charts', 'drawings']));
+  });
+});

test/spreadsheet-xlsx-core.test.ts

@@ -17,6 +17,7 @@ type Core = {
     limits: Record<string, number>
   ): { push(chunk: Uint8Array, final: boolean): void };
   buildAdmittedArchive(admission: unknown, zip: typeof fflate): Uint8Array;
+  repairExcelJsModel(model: unknown): unknown;
   excelJsEntryName(name: string): string;
   parseCellRef(ref: string): { row: number; col: number } | null;
   deriveExtent(cells: string[], merges: string[]): { rows: number; cols: number };
@@ -803,3 +804,127 @@ describe('spreadsheet XLSX colour resolution', () => {
     expect(core.resolveCellColors({ argb: 'bogus' }, { argb: '' })).toEqual({});
   });
 });
+
+// ExcelJS 4.4.0's parsed model at the moment it reconciles, reduced to the parts
+// repairExcelJsModel reads. `excelShaped()` is what Excel itself writes (and
+// ExcelJS links cleanly); the openpyxl cases below are what made it throw.
+const REL = 'http://schemas.openxmlformats.org/officeDocument/2006/relationships/';
+const rel = (Id: string, type: string, Target: string) => ({ Id, Type: REL + type, Target });
+type Model = Record<string, any>;
+
+function excelShaped(): Model {
+  return {
+    worksheets: [
+      {
+        sheetNo: '1',
+        tables: [{ rId: 'rId3' }],
+        drawing: { rId: 'rId4' },
+        hyperlinks: [{ rId: 'rId5', address: 'A1' }, { address: 'B2' }],
+      },
+    ],
+    worksheetRels: {
+      '1': [
+        rel('rId1', 'comments', '../comments1.xml'),
+        rel('rId2', 'vmlDrawing', '../drawings/vmlDrawing1.vml'),
+        rel('rId3', 'table', '../tables/table1.xml'),
+        rel('rId4', 'drawing', '../drawings/drawing1.xml'),
+        rel('rId5', 'hyperlink', 'https://example.com'),
+      ],
+    },
+    comments: { '../comments1.xml': { comments: [{ ref: 'B2' }] } },
+    vmlDrawings: { '../drawings/vmlDrawing1.vml': { comments: [{}] } },
+    tables: { '../tables/table1.xml': { name: 'Sales' } },
+    drawings: { drawing1: { anchors: [] } },
+    drawingRels: { drawing1: [] },
+  };
+}
+
+describe('repairExcelJsModel: links ExcelJS would dereference unchecked', () => {
+  it('leaves a model Excel itself wrote untouched', () => {
+    const model = excelShaped();
+    const before = structuredClone(model);
+    core.repairExcelJsModel(model);
+    expect(model).toEqual(before);
+  });
+
+  it("re-points openpyxl's absolute table Target to the key ExcelJS stored that table under", () => {
+    const model = excelShaped();
+    model.worksheetRels['1'][2].Target = '/xl/tables/table1.xml';
+    core.repairExcelJsModel(model);
+    expect(model.worksheetRels['1'][2].Target).toBe('../tables/table1.xml');
+    expect(model.worksheets[0].tables).toEqual([{ rId: 'rId3' }]);
+  });
+
+  it('drops comment and note links that resolve to nothing (openpyxl names the parts its own way)', () => {
+    const model = excelShaped();
+    model.worksheetRels['1'][0].Target = '/xl/comments/comment1.xml';
+    model.worksheetRels['1'][1].Target = '/xl/drawings/commentsDrawing1.vml';
+    model.comments = {};
+    model.vmlDrawings = {};
+    core.repairExcelJsModel(model);
+    expect(model.worksheetRels['1'].map((r: { Id: string }) => r.Id)).toEqual(['rId3', 'rId4', 'rId5']);
+  });
+
+  it('drops a note link whose VML has no comments list, as ExcelJS would index into it', () => {
+    const model = excelShaped();
+    model.vmlDrawings['../drawings/vmlDrawing1.vml'] = {};
+    core.repairExcelJsModel(model);
+    expect(model.worksheetRels['1'].some((r: { Id: string }) => r.Id === 'rId2')).toBe(false);
+  });
+
+  it('drops a drawing ExcelJS parsed to nothing and unhooks the sheet from it (openpyxl charts)', () => {
+    const model = excelShaped();
+    model.drawings.drawing1 = undefined;
+    model.worksheetRels['1'][3].Target = '/xl/drawings/drawing1.xml';
+    core.repairExcelJsModel(model);
+    expect(model.drawings).toEqual({});
+    expect(model.drawingRels).toEqual({});
+    expect(model.worksheets[0].drawing).toBeUndefined();
+  });
+
+  it('keeps a drawing link whose Target ExcelJS does not try to resolve', () => {
+    const model = excelShaped();
+    model.worksheetRels['1'][3].Target = 'drawing-without-extension';
+    core.repairExcelJsModel(model);
+    expect(model.worksheets[0].drawing).toEqual({ rId: 'rId4' });
+  });
+
+  it('unhooks a tablePart, drawing or hyperlink whose rId names no relationship', () => {
+    const model = excelShaped();
+    model.worksheets[0].tables = [{ rId: 'missing' }, { rId: 'rId5' }];
+    model.worksheets[0].drawing = { rId: 'missing' };
+    model.worksheets[0].hyperlinks = [{ rId: 'missing', address: 'A1' }, { address: 'B2' }];
+    core.repairExcelJsModel(model);
+    expect(model.worksheets[0].tables).toEqual([]);
+    expect(model.worksheets[0].drawing).toBeUndefined();
+    expect(model.worksheets[0].hyperlinks).toEqual([{ address: 'B2' }]);
+  });
+
+  it('drops a table rel whose part ExcelJS never stored, and its tablePart with it', () => {
+    const model = excelShaped();
+    model.worksheetRels['1'][2].Target = '/xl/somewhere/else.xml';
+    model.tables = { '../tables/table1.xml': undefined };
+    core.repairExcelJsModel(model);
+    expect(model.tables).toEqual({});
+    expect(model.worksheetRels['1'].some((r: { Id: string }) => r.Id === 'rId3')).toBe(false);
+    expect(model.worksheets[0].tables).toEqual([]);
+  });
+
+  it('never resolves a Target or rId through Object.prototype', () => {
+    const model = excelShaped();
+    model.worksheetRels['1'][0].Target = '__proto__';
+    model.worksheetRels['1'][2].Target = 'constructor';
+    model.worksheets[0].tables = [{ rId: 'toString' }];
+    model.worksheets[0].hyperlinks = [{ rId: 'hasOwnProperty', address: 'A1' }];
+    core.repairExcelJsModel(model);
+    expect(model.worksheetRels['1'].map((r: { Id: string }) => r.Id)).toEqual(['rId2', 'rId4', 'rId5']);
+    expect(model.worksheets[0].tables).toEqual([]);
+    expect(model.worksheets[0].hyperlinks).toEqual([]);
+  });
+
+  it('tolerates a model with no worksheets, rels or part maps', () => {
+    expect(() => core.repairExcelJsModel({})).not.toThrow();
+    expect(() => core.repairExcelJsModel({ worksheets: [null, { sheetNo: '9' }] })).not.toThrow();
+    expect(core.repairExcelJsModel(null)).toBeNull();
+  });
+});