Skip to content

Commit d38ad7f

Browse files
Trumpclaude
andauthored
fix(rest): type the two production exceljs dynamic imports behind one named accessor (#13452)
* fix(rest): type the two production exceljs dynamic imports behind one named accessor Both places `packages/rest` production source reached exceljs bound the module as `const ExcelJS: any`, so `parseXlsxToRows` (the whole .xlsx import path) and `createXlsxStream` (the streaming .xlsx export path) built workbooks, read worksheets, iterated rows and read cells through a value tsc knew nothing about. A misspelled method, a wrong arity or a property exceljs renamed was not a compile error, only a runtime fault in a deployed import or export. `src/xlsx-module.ts` is now the single binding site both paths share. It keeps the load lazy (everything is either a type, erased at emit, or inside the async accessor), and it states the trade it accepts: typing the dynamic import pulls exceljs's declarations -- including the module-local `Buffer` shim -- into production modules that previously kept them out. The cost drops from "the whole path is unchecked" to "one named assertion with its reason written next to it". That assertion, `asXlsxLoadInput`, takes `Buffer` and not `Buffer | ArrayBuffer` deliberately: `ArrayBuffer` is already assignable to exceljs's shim, so that arm reaches `load` unasserted and stays checked. `wb.getWorksheet(sheet as any)` loses its cast for the same reason -- the real signature accepts `string | number` as it stands. Typing-only. No runtime behaviour change: the interop expression is the one the call sites already ran (awaited once rather than twice -- the second await resolved from the module cache to the identical record), and `Row.values`'s non-array shape iterates zero times before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k * chore(changeset): @objectstack/rest patch for the production exceljs typing Measured rather than assumed: the accessor reaches the published artifact (`loadExcelJs` greps 3 in dist/index.js and 3 in dist/index.cjs, positive control `RestServer` = 36), so this PR releases something and `skip-changeset` would be false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k * test(rest): pin the accessor's interop and both parseXlsxToRows arms; re-anchor the isSystem census The typed accessor's TYPE half is asserted by tsc; its runtime half is not, and two things there can rot silently. `xlsx-module.test.ts` pins both: the CommonJS interop (drop either half of `.default ?? namespace` and `Workbook` is undefined, visible only in a deployed import/export), and both arms of `parseXlsxToRows(buffer: Buffer | ArrayBuffer, ...)` -- the Node `Buffer` arm that carries the assertion and the `ArrayBuffer` arm that stays checked without one -- driven against bytes the accessor itself wrote. `check:system-context-census` went red on this branch and the cause is this branch: the one import line added to `rest-server.ts` shifted every elevation read below it by one, so nine anchors on `content/docs/permissions/system-context.mdx` pointed one line short. Measured, not assumed -- with `rest-server.ts` alone restored to the merge base the gate reads `OK - 109 elevation read sites ... 145 anchors resolve`. `--fix` refuses this one by design (it counts 8 page read-anchors against 6 census sites and calls that a population change, not a shift), so the nine anchors are bumped by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k * docs(rest): the package-door census says which `.catch` is load-bearing, and stops claiming the swallow is documented nowhere Comment-only. Item 3 of the 2026-08-29 ride-along ruling: the census file is where a reader of this door lands, and it named two `.catch(() => undefined)` sites without saying that a production fault reaches NEITHER of them as a rejection. `computeExecCtx` wraps its whole body in `try { ... } catch { return undefined; }`, so the resolve FULFILS with `undefined` and the fault-to-anonymous conversion has already happened one level below -- which is exactly the mistake the ruling names, "stops a future reader from removing the wrong `.catch` expecting a fault to surface". It also records that this first net is NOT per-door: the two `.catch`es are per-consumer (16 of them in rest-server.ts) while computeExecCtx's catch is one site every consumer inherits. Both facts are cited, not restated -- they are measured in package-door-execctx-fault-reading.test.ts, package-door-execctx-fault-reachability.test.ts and execctx-consumer-census.test.ts, and a second copy is a second thing to drift. The same paragraph's closing note said the swallow is "documented at NEITHER site". That is now half stale and was corrected in place rather than left adjacent to a fresh correction: rest-server.ts's resolvePackageRouteExecutionContext carries the reading (the second-net point included); the package-routes.ts site still carries none. Comment-only proven with scripts/js-comment-mask.mjs, on a comparator calibrated in BOTH directions: code identity 9b977535ecc0592f before and after, while a one-token code change to the same file DIFFERS and a comment word change does not. Raw bytes differ (5dbceb5dddb84287 -> d74dac3166529e9f), so the green is not "nothing happened". rest-server.ts is untouched, so the nine hand-bumped anchors in content/docs/permissions/system-context.mdx cannot have moved; check:system-context-census re-reads OK over 109 sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 370524d commit d38ad7f

7 files changed

Lines changed: 283 additions & 13 deletions

File tree

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
"@objectstack/rest": patch
3+
---
4+
5+
fix(rest): type the two production exceljs dynamic imports behind one named accessor
6+
7+
Both places `@objectstack/rest` production source reached exceljs bound the
8+
module as `const ExcelJS: any`, so `parseXlsxToRows` (the whole .xlsx import
9+
path) and `createXlsxStream` (the streaming .xlsx export path) built workbooks,
10+
read worksheets, iterated rows and read cells through a value tsc knew nothing
11+
about. A misspelled method, a wrong argument arity or a property exceljs
12+
renamed was not a compile error — it surfaced, if at all, as a runtime fault in
13+
a deployed import or export.
14+
15+
`src/xlsx-module.ts` is now the single binding site both paths share, and it
16+
keeps the load lazy: everything in it is either a type (erased at emit) or
17+
inside the async accessor, so a CSV or JSON import still never pays to load
18+
exceljs.
19+
20+
**Typing-only — no runtime behaviour change, and no change to the published API
21+
surface.** `parseXlsxToRows` keeps its exact signature
22+
(`(buffer: Buffer | ArrayBuffer, mapping?, sheet?)`), the package's exports are
23+
unchanged, and the accessor is internal. The interop expression is the one the
24+
call sites already ran (awaited once rather than twice — the second
25+
`await import('exceljs')` resolved from the module cache to the identical
26+
record), and the non-array shape of `Row.values` iterates zero times before and
27+
after. `@objectstack/rest`'s 160 test files / 2698 tests pass unchanged.
28+
29+
This is a `patch` rather than a `skip-changeset` because it was measured to
30+
reach the published artifact: `loadExcelJs` greps 3 in `dist/index.js` and 3 in
31+
`dist/index.cjs` (`asXlsxLoadInput` 2 and 2), against a positive control of
32+
`RestServer` = 36.

content/docs/permissions/system-context.mdx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ not on any flag.
6464
## How the flag is set
6565

6666
`isSystem` is **server-constructed and never client-supplied**. Inbound HTTP
67-
cannot set it (`packages/rest/src/rest-server.ts:1234`, `:1263`), and neither
67+
cannot set it (`packages/rest/src/rest-server.ts:1235`, `:1264`), and neither
6868
can an action body (`packages/runtime/src/domains/actions.ts:404`). It is
6969
written by internal callers only, as an option on the engine call:
7070

@@ -103,7 +103,7 @@ that silently does not happen.
103103
| 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:246` |
104104
| 15 | **Read-audit rows are not written** | plugin-audit | Lose: the "a person opened this record" trail. `sudo()` keeps the caller's `userId`, so this flag is the only thing separating a human read from a platform one | `read-audit.ts:556` |
105105
| 16 | Approval snapshot payload redaction skipped | plugin-approvals | Get: the whole snapshot on `find` / `findOne` — the audit/replay channel. Lose: field-visibility redaction over approval payloads | `payload-redaction-middleware.ts:115` |
106-
| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1266` |
106+
| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1267` |
107107

108108
### 2. Write pipeline and data integrity
109109

@@ -158,7 +158,7 @@ The largest single consumer — **20 of the 109 sites**.
158158
|:--|:---|:---|:---|:---|
159159
| 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `action-execution.ts:136` |
160160
| 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:399` |
161-
| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4270`, `:5633`, `:5865`, `:6210`, `:6403` |
161+
| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4271`, `:5634`, `:5866`, `:6211`, `:6404` |
162162
| 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` |
163163
| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:411`, `domains/ai.ts:60`, `domains/automation.ts:982`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:246`, `external-datasource-routes.ts:302`, `package-routes.ts:92` |
164164
| 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` |
@@ -199,7 +199,7 @@ assuming `isSystem` covers it is a documented source of bugs.
199199
| "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1451` (#3493 / #6640) |
200200
| "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280``281` |
201201
| "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:273` |
202-
| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1234`, `:1263`; `domains/actions.ts:404` |
202+
| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1235`, `:1264`; `domains/actions.ts:404` |
203203

204204
---
205205

packages/rest/src/import-prepare.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
type ExportFieldMeta,
1414
} from './export-format.js';
1515
import { resolveNamedMapping, applyMappingToRows, type MappingArtifactLike } from './import-mapping.js';
16+
import { asXlsxLoadInput, loadExcelJs } from './xlsx-module.js';
1617

1718
/**
1819
* Minimal RFC-4180-style CSV parser used by the bulk-import endpoint
@@ -138,15 +139,23 @@ export async function parseXlsxToRows(
138139
mapping: Record<string, string> = {},
139140
sheet?: string | number,
140141
): Promise<Array<Record<string, any>>> {
141-
const ExcelJS: any = (await import('exceljs')).default ?? (await import('exceljs'));
142+
const ExcelJS = await loadExcelJs();
142143
const wb = new ExcelJS.Workbook();
143-
await wb.xlsx.load(buffer);
144-
const ws = sheet !== undefined ? wb.getWorksheet(sheet as any) : wb.worksheets[0];
144+
// The assertion is on the Node `Buffer` arm ONLY. `ArrayBuffer` is already
145+
// assignable to the module-local shim exceljs declares for this parameter,
146+
// so that arm reaches `load` unasserted and stays checked; asserting the
147+
// whole union would switch that check off. See `xlsx-module.ts`.
148+
await wb.xlsx.load(buffer instanceof ArrayBuffer ? buffer : asXlsxLoadInput(buffer));
149+
const ws = sheet !== undefined ? wb.getWorksheet(sheet) : wb.worksheets[0];
145150
if (!ws) return [];
146151

147152
const cells: string[][] = [];
148-
ws.eachRow({ includeEmpty: false }, (row: any) => {
149-
const values = row.values as any[]; // 1-based; index 0 is unused
153+
ws.eachRow({ includeEmpty: false }, (row) => {
154+
// exceljs declares `Row.values` as the 1-based array form (index 0
155+
// unused) OR a keyed object; a workbook parsed from bytes yields the
156+
// array. The keyed shape iterates zero times either way — its `.length`
157+
// was `undefined` before, so the loop below never ran for it.
158+
const values = Array.isArray(row.values) ? row.values : [];
150159
const line: string[] = [];
151160
for (let c = 1; c < values.length; c++) line.push(xlsxCellToString(values[c]));
152161
cells.push(line);

packages/rest/src/package-door-declared-code.test.ts

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,38 @@
100100
* itself and the handler's `try` catches it) — and the production wrapper
101101
* above has no statement that can make one.
102102
*
103+
* ⭐ WHICH `.catch` is load-bearing: NEITHER of the two named above. A
104+
* production fault never arrives at either one AS A REJECTION.
105+
* `computeExecCtx` — the private body behind `resolveExecCtx` — wraps its
106+
* WHOLE body in `try { … } catch { return undefined; }`, so a faulting
107+
* resolve FULFILS with `undefined` rather than rejecting, and the
108+
* fault-to-anonymous conversion has already happened one level BELOW by the
109+
* time either `.catch(() => undefined)` is reached. ⇒ removing either of them
110+
* would not by itself surface a production fault: a reader who deletes one
111+
* expecting a 5xx to appear measures no change and concludes the wrong thing
112+
* about where the conversion lives. That is the whole reason this paragraph
113+
* exists.
114+
*
115+
* ⚠️ And that first net is NOT per-door, unlike the two above it.
116+
* `resolveExecCtx(environmentId, req).catch(() => undefined)` is written
117+
* per-consumer — 16 times in `rest-server.ts` as this file is written — while
118+
* `computeExecCtx`'s catch is ONE site whose conversion every one of those
119+
* consumers inherits. So what this door reads is not a property OF this door,
120+
* and a census of the other consumers is a separate file rather than a
121+
* section here.
122+
*
123+
* ⛔ Both facts are MEASURED ELSEWHERE and cited here rather than restated —
124+
* a second copy is a second thing to drift, and this file's own convention is
125+
* to state a reason once and point at it:
126+
* - `package-door-execctx-fault-reading.test.ts` — that the production
127+
* supplier really does fulfil rather than reject, read per class.
128+
* - `package-door-execctx-fault-reachability.test.ts` — that no degraded
129+
* class reaches the wrapper as a rejection, against a control showing the
130+
* witness CAN report one.
131+
* - `execctx-consumer-census.test.ts` — the per-site census over every
132+
* `resolveExecCtx` consumer, each row DRIVEN rather than read off the
133+
* shape.
134+
*
103135
* ⚠️ Nor can an embedder reach it: `registerPackageRoutes` and
104136
* `PackageRoutesOptions` are NOT exported from `packages/rest/src/index.ts`
105137
* (the package publishes a single `.` entry, and `direct-mount-composition.ts`
@@ -116,8 +148,11 @@
116148
* ⛔ Whether that double swallow SHOULD exist at all is a different
117149
* question — it would change what a public door emits — and it is open
118150
* at #12537, deliberately not answered here. Note for whoever takes it: the
119-
* swallow is documented at NEITHER site, so "deliberate" is not established by
120-
* the code as it stands.
151+
* swallow is now documented at ONE of the two sites, not neither —
152+
* `rest-server.ts`'s `resolvePackageRouteExecutionContext` carries the
153+
* reading, the second-net correction above included — while the
154+
* `package-routes.ts` site still carries none. So "deliberate" is established
155+
* by the code for the first site and still is not for the second.
121156
*
122157
* Section 5 is the second fact stated as a test: a REAL `ObjectQL`, a REAL
123158
* `ObjectStackProtocolImplementation` and a failing driver, driven through the

packages/rest/src/rest-server.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,7 @@ import {
194194
} from './export-format.js';
195195
import { runImport } from './import-runner.js';
196196
import { prepareImportRequest } from './import-prepare.js';
197+
import { loadExcelJs, type Worksheet } from './xlsx-module.js';
197198
import { enrichOpenApiWithEndpoints } from './openapi-endpoints.js';
198199
import { buildBuiltinPaths } from './openapi-builtin-paths.js';
199200
import {
@@ -605,11 +606,11 @@ function rowsToCsv(
605606
* module's static graph.
606607
*/
607608
async function createXlsxStream(res: any, useStyles = false): Promise<{
608-
ws: any;
609+
ws: Worksheet;
609610
finalize: () => Promise<void>;
610611
}> {
611612
const { PassThrough } = await import('node:stream');
612-
const ExcelJS: any = (await import('exceljs')).default ?? (await import('exceljs'));
613+
const ExcelJS = await loadExcelJs();
613614

614615
const passthrough = new PassThrough();
615616
const done = new Promise<void>((resolve, reject) => {
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Runtime pins for `xlsx-module.ts` — the single typed, lazily-loaded exceljs
5+
* binding both production xlsx paths share.
6+
*
7+
* The TYPE half of that module (workbooks, worksheets, rows and cells are now
8+
* inside tsc's reach) is asserted by tsc itself and cannot be asserted from
9+
* here. What CAN rot at runtime is the part that is not a type:
10+
*
11+
* - the CommonJS interop (`.default ?? namespace`) the accessor performs. A
12+
* "simplification" that drops either half returns a namespace whose
13+
* `Workbook` is `undefined`, and the failure surfaces only in a deployed
14+
* import or export.
15+
* - both arms of `parseXlsxToRows(buffer: Buffer | ArrayBuffer, …)`. Only the
16+
* Node `Buffer` arm carries the type assertion; the `ArrayBuffer` arm is
17+
* type-checked as it stands. Neither is allowed to change behaviour, so
18+
* both are driven here against bytes the accessor itself produced.
19+
*/
20+
21+
import { describe, it, expect } from 'vitest';
22+
import { loadExcelJs } from './xlsx-module.js';
23+
import { parseXlsxToRows } from './import-prepare.js';
24+
25+
/** A two-row sheet, written through the accessor's own namespace. */
26+
async function writeFixture(): Promise<Buffer> {
27+
const ExcelJS = await loadExcelJs();
28+
const wb = new ExcelJS.Workbook();
29+
const ws = wb.addWorksheet('Sheet1');
30+
ws.addRow(['id', 'title', 'score']);
31+
ws.addRow(['a1', 'first', 7]);
32+
ws.addRow(['a2', 'second', 9]);
33+
return Buffer.from(await wb.xlsx.writeBuffer());
34+
}
35+
36+
describe('loadExcelJs', () => {
37+
it('resolves the exceljs namespace with the interop the call sites used to do inline', async () => {
38+
const ExcelJS = await loadExcelJs();
39+
// `.default ?? namespace`: drop either half and one of these is undefined.
40+
expect(typeof ExcelJS.Workbook).toBe('function');
41+
expect(typeof ExcelJS.stream.xlsx.WorkbookWriter).toBe('function');
42+
expect(new ExcelJS.Workbook().worksheets).toEqual([]);
43+
});
44+
45+
it('returns the same module record on repeat calls — the load stays cached, not re-fetched', async () => {
46+
expect(await loadExcelJs()).toBe(await loadExcelJs());
47+
});
48+
});
49+
50+
describe('parseXlsxToRows keeps both arms of its Buffer | ArrayBuffer parameter', () => {
51+
it('reads the Node `Buffer` arm — the one arm the exceljs shim forces an assertion on', async () => {
52+
const rows = await parseXlsxToRows(await writeFixture());
53+
expect(rows).toEqual([
54+
{ id: 'a1', title: 'first', score: '7' },
55+
{ id: 'a2', title: 'second', score: '9' },
56+
]);
57+
});
58+
59+
it('reads the `ArrayBuffer` arm — the arm that stays type-checked, unasserted', async () => {
60+
const bytes = await writeFixture();
61+
const arrayBuffer = bytes.buffer.slice(
62+
bytes.byteOffset,
63+
bytes.byteOffset + bytes.byteLength,
64+
) as ArrayBuffer;
65+
expect(arrayBuffer).toBeInstanceOf(ArrayBuffer);
66+
const rows = await parseXlsxToRows(arrayBuffer);
67+
expect(rows).toEqual([
68+
{ id: 'a1', title: 'first', score: '7' },
69+
{ id: 'a2', title: 'second', score: '9' },
70+
]);
71+
});
72+
73+
it('honours the sheet selector through the typed `getWorksheet` (its `as any` is gone)', async () => {
74+
const ExcelJS = await loadExcelJs();
75+
const wb = new ExcelJS.Workbook();
76+
wb.addWorksheet('Empty'); // decoy first sheet
77+
const ws = wb.addWorksheet('Data');
78+
ws.addRow(['id', 'title']);
79+
ws.addRow(['x1', 'from-named-sheet']);
80+
const bytes = Buffer.from(await wb.xlsx.writeBuffer());
81+
expect(await parseXlsxToRows(bytes, {}, 'Data')).toEqual([
82+
{ id: 'x1', title: 'from-named-sheet' },
83+
]);
84+
});
85+
});

0 commit comments

Comments
 (0)