Skip to content

fix(rest): type the two production exceljs dynamic imports behind one named accessor - #13452

Merged
os-trump merged 7 commits into
mainfrom
claude/issue-13426-production-exceljs-typing
Aug 30, 2026
Merged

fix(rest): type the two production exceljs dynamic imports behind one named accessor#13452
os-trump merged 7 commits into
mainfrom
claude/issue-13426-production-exceljs-typing

Conversation

@os-trump

@os-trump os-trump commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Fixes #13426
Fixes #12537

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 argument arity or a property exceljs renamed was not a
compile error — it surfaced, if at all, as a runtime fault in a deployed import or
export.

The accessor — one named binding site, shared by both production paths

packages/rest/src/xlsx-module.ts is now the only production file in this package
that names exceljs. It exports loadExcelJs() (the lazy, typed namespace),
asXlsxLoadInput() (the one assertion), and re-exports the exceljs types the two
call sites need. The parameter type is read off the dependency's own signature —
the Parameters[0] of exceljs's Xlsx load member — rather than spelled by
hand, the technique the test-layer loader on open PR #13425 (card #13378)
introduced. If exceljs ever drops its shim, that alias resolves to Node's Buffer
and the assertion quietly becomes a no-op rather than a lie.

The lazy import is preserved and is still the point. Everything in the module
is either a type (erased at emit) or inside the async accessor, so a CSV or JSON
import still never pays to load exceljs. Nothing here is a static import.

The Buffer arm, and only the Buffer arm

asXlsxLoadInput takes Buffer, not Buffer | ArrayBuffer. ArrayBuffer is
already assignable to exceljs's module-local shim, so that arm reaches load
unasserted and stays genuinely checked:

await wb.xlsx.load(buffer instanceof ArrayBuffer ? buffer : asXlsxLoadInput(buffer));

A blanket assertion over the union would have switched off type checking that works
today. Both arms are pinned at runtime by the new test file. In the same spirit,
wb.getWorksheet(sheet as any) loses its cast — the real signature accepts a string
or a number as it stands.

The trade, written into the accessor's comment

Typing the dynamic import does pull exceljs's declarations — including the
module-local Buffer shim — into production modules that previously kept them out.
That is accepted deliberately, and it is a trade down: before this branch the
cost was the whole path is unchecked; after it the cost is one named assertion
with its reason written next to it
, in a place a future exceljs release can retire
in one edit. The accessor's docblock states it.

Proof that the typing bites

A green typecheck alone proves nothing here, so the same deliberate mutation was
driven through both the typed code and the base code — a misspelled
wb.xlsx.load on the import path and a misspelled ws.addRow at the export
caller, injection confirmed on disk by grep counts and blob hashes, restored and
verified by blob hash plus an empty git diff HEAD.

leg tsc exit diagnostics
mutation on this branch's typed code 2 4
identical mutation on the merge-base any code 0 0
src/import-prepare.ts(148,19): error TS2339: Property 'laod' does not exist on type 'Xlsx'.
src/rest-server.ts(8152,42): error TS2551: Property 'addRowe' does not exist on type 'Worksheet'. Did you mean 'addRow'?
src/rest-server.ts(8156,52): error TS2551: Property 'addRowe' does not exist on type 'Worksheet'. Did you mean 'addRow'?
src/rest-server.ts(8197,38): error TS2551: Property 'addRowe' does not exist on type 'Worksheet'. Did you mean 'addRow'?

The base leg is the load-bearing half: zero diagnostics for the identical mutation
is the measurement that the path really was outside the type system, and is now
inside it.

Anti-vacuity. tsc --noEmit --listFiles in packages/rest lists 452 files and
contains all three edited production files; exceljs's index.d.ts is in the program
(positive control that the declarations are live); a .test.ts count of 0 is the
negative control for that program's own exclude. The ablation needed no rebuild
and is not exempt from the rule: the mutated files are read from src/ by the same
program that renders the verdict, which is exactly what --listFiles shows.

The census repair this branch owed

check:system-context-census went red here, and the cause was 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 rather than 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 shape 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. Gate now green.

Changeset — measured, not copied

@objectstack/rest gets a patch. The accessor does reach the published
artifact: loadExcelJs greps 3 in dist/index.js and 3 in dist/index.cjs
(asXlsxLoadInput 2 and 2), against a positive control of RestServer = 36 and a
negative control of loadXlsxWorkbook = 0. That is the opposite reading from the
test-layer helper on PR #13425, whose skip-changeset was earned by a grep of 0 —
so it is not borrowed here.

It is nevertheless typing-only: parseXlsxToRows keeps its exact signature, the
package's exports are unchanged, loadExcelJs greps 0 in dist/index.d.ts, and the
suite is green unchanged (2698 tests before this branch's own new file, 2703 with
it). The interop expression is the one the call sites already ran, awaited once
rather than twice — the second await import('exceljs') resolved from the module
cache to the identical record.

Discharging #12537 — items 1 and 2 already on main, item 3 added here

The 2026-08-29 ruling (verbatim 「同意」) is a three-part ride-along, to be carried
by the next rest-server.ts vehicle and explicitly never its own PR. This PR is that
vehicle. My first pass reported the card as entirely stale; that was two-thirds right,
and the third part really was missing.

ruling item discharged where check it at
annotate the resolveExecutionContext section as a test injection point already on main package-door-declared-code.test.ts:284
correct repo-wide "four seams" to "three" already on main same file, :71 and :260
carry the computeExecCtx correction into the census file this PR same file, the new block above "Nor can an embedder reach it"

Item 3's absence was a reading, not an instrument failure — the zeros were taken with
positive controls in the same file: computeExecCtx 0 and
second net|absorbed|one level|outer catch 0, against resolveExecCtx 2, TEST-ONLY
2 and Seam census 7.

What was added, and why it is that file's problem. The census file names two
.catch(() => undefined) sites and told a reader that a production resolver's
"rejections are swallowed twice". 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. Deleting
either .catch expecting a fault to surface therefore changes nothing — the mistake
the ruling exists to prevent. The new block also records that this first net is not
per-door
: the two .catches are per-consumer (16 in rest-server.ts) while
computeExecCtx's catch is one site every consumer inherits. Both facts are cited,
not restated
package-door-execctx-fault-reading.test.ts,
package-door-execctx-fault-reachability.test.ts and execctx-consumer-census.test.ts
own the measurements, and a second copy is a second thing to drift. The citation ran
siblings to census file only; now it runs both ways.

One adjacent sentence corrected in the same paragraph. It read "the swallow is
documented at NEITHER site". That is now half stale, and leaving a false claim beside a
fresh correction would be worse than the correction is worth:
rest-server.ts's resolvePackageRouteExecutionContext carries the reading (the
second-net point included); the package-routes.ts site still carries none. The
sentence now says exactly that.

⛔ Option A (propagating the resolver fault as a 5xx) is not done here — declined by
the maintainer — and the swallow behaviour question was split out to #13255, so the
record correction is all that remained on this card.

Comment-only, on a two-way-calibrated instrument. The change to
package-door-declared-code.test.ts is 37 added and 2 removed comment lines and no
code. Proven with scripts/js-comment-mask.mjs (stripComments), comparing code
identity after excluding whitespace-only differences:

RAW bytes        before=5dbceb5dddb84287  after=d74dac3166529e9f   DIFFER  (the edit landed at all)
CODE identity    before=9b977535ecc0592f  after=9b977535ecc0592f   IDENTICAL => comment-only
  calibration: one CODE token changed in the same file   => DIFFER        (the instrument can fail)
  calibration: one COMMENT word changed                  => IDENTICAL     (and does not over-report)
  calibration: an added COMMENT LINE                     => IDENTICAL

⚠️ Recorded because it nearly produced a false red: comparing stripComments output
raw says NOT comment-only here, and it is wrong for a reason worth writing down —
that function keeps every newline by design so line numbers survive, so N added comment
lines add N newlines to its output. The measured delta was +35 newlines, +0 spaces, +0
non-whitespace characters, with all 338 non-empty code lines identical in content and
order. The comparator that answers "did any code change" has to exclude whitespace; the
raw one answers "did the line map move".

rest-server.ts is untouched by this commit, so the nine hand-bumped anchors in
content/docs/permissions/system-context.mdx cannot have moved — and
check:system-context-census re-reads
OK — 109 elevation read sites in 20 packages across 45 files, all anchored at the
final head.

Two corrections to my own earlier report

Verification

All at 9d33b427c6, this branch's head, with the current origin/main (71627f7b4e)
merged in before the gate union was derived.

  • pnpm --filter @objectstack/rest typecheck — exit 0;
    check:test-typecheck: OK — @objectstack/rest's test layer compiles under packages/rest/tsconfig.test.json; 3 file(s) / 4 error(s) held in test-typecheck-debt.json
    (that ledger reads 4 where this PR's earlier revision read 5 — PR fix(rest): state the exceljs Buffer-shim assertion once, in a typed test loader #13425 shrank it on
    main and the merge brought that in; shrink-only, and not this branch's doing)
  • pnpm --filter @objectstack/rest exec vitest run --maxWorkers=2
    Test Files 161 passed (161), Tests 2703 passed (2703)
  • pnpm lint (eslint . --no-inline-config, whole repo) — exit 0
  • pnpm check:nul-bytesOK (scanned 7450 text file(s) … no raw ASCII control bytes),
    plus a control-byte grep over this PR's seven files with zero hits
  • Gate union: 44 path-matched families plus 4 convention-triggered ones, 48 in total:
    44 run green at this head, 4 recorded NOT MEASURED below.
    Among the green:
    check:system-context-census
    (OK — 109 elevation read sites in 20 packages across 45 files, all anchored),
    check:engine-double-contract, check:where-matcher, check:query-options-erasure,
    check:type-check-coverage, check:dispatcher-error-vocabulary,
    check:comment-mask-adoption, check:changeset-gate-self-tests,
    check:route-envelope, check:authz-resolver, and the docs families the census edit
    pulled in.

⚠️ The gate derivation was silently stale, and only an independent check caught it.
Re-deriving on the tree as handed to me printed no staleness banner and exited 0 while
origin/main was two commits ahead — one of them #13447, which rewrites
dispatch-gates.mjs's own watch-hint machinery (220 lines), i.e. exactly the code that
decides which families a path derives. The banner's silence is not evidence of
freshness; git rev-list --count HEAD..origin/main is. After merging, the re-derived
list came out identical (44 + 4), so nothing was missed this time — but that could
only be known by re-deriving, not by trusting the exit code.

NOT MEASURED — four, none of them a red, each in the gate's own words:

  • check:dual-build-cjs-loadsPREREQUISITE NOT MET — this gate reads built output, and some package has no dist/. (36 packages: 8 named, "… 28 more"). Repo-scale build; CI owns it.
  • check:type-check-debt — refuses outright: --re-measure cannot run: 26 workspace dependenc(ies) of the ledgered packages have no built type entry point on disk, and its own text says a number taken from here would measure a different world. Its two nearby halves were measured green: check:type-check-coverage (the structural half) and this package's own test-layer ratchet inside typecheck.
  • scripts/check-test-completeness.mjs — exit 3, PREREQUISITE NOT MET — this gate grades a saved turbo run test log, and no log was named. Its own help text names this exact invocation and says to record NOT MEASURED.
  • scripts/pm/check-half-states.mjs — exit 3, PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential. (pnpm check:pm-half-states, a different invocation, ran green.)

⚠️ State on this PR I did not set and have not touched: it is draft: false. I read
that rather than assumed it, and preserved it on this edit instead of reverting it —
flipping someone else's ready state is not mine to do. Flagging it only so the record
says who did what.

Generated by Claude Code

claude added 4 commits August 30, 2026 12:16
… 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
…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
… 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
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

6 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 71627f7b4e9f7919784dc21006fdb6b2720104b8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0412f951811d5189dadbe80a22daa1085cd22dd0 — the merge of head 9d33b427c61e2287a2cee7db2643cc660ff5afe1 into base 71627f7b4e9f7919784dc21006fdb6b2720104b8, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 0412f951811d5189dadbe80a22daa1085cd22dd0 && git checkout 0412f951811d5189dadbe80a22daa1085cd22dd0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 71627f7b4e9f7919784dc21006fdb6b2720104b8 9d33b427c61e2287a2cee7db2643cc660ff5afe1 && git checkout -B drift-repro 71627f7b4e9f7919784dc21006fdb6b2720104b8 && git merge --no-ff 9d33b427c61e2287a2cee7db2643cc660ff5afe1

node scripts/docs-audit/affected-docs.mjs --json 71627f7b4e9f7919784dc21006fdb6b2720104b8

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

os-trump and others added 3 commits August 30, 2026 20:55
…ng, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment