ci: ❌ fail the build when a pkg.scripts entry matches no file

[j-base64]

TL;DR: A pkg.scripts entry that matches no file is silently dropped from the packaged binary,
and only crashes with MODULE_NOT_FOUND at runtime when a specific code path is exercised. Without
exercising that path, it's easy to assume a feature is in the build when it's not (it was the case for DocumentServer#267).

This PR adds a small build guard that fails when any component's pkg.scripts entry resolves to no
file, and runs it before the pkg step in both the production image build and the e2e CI job.

🔥Ready for review; it stays a draft only because e2e is correctly red until #46 adds the editorDataRedis module or we decide to remove the stale entries (+details below)

Details

A component's package.json pkg.scripts block reads like the list of modules that ship in the
build, so an entry there implies the module is present. It may not be: @yao-pkg/pkg bundles the
files listed under pkg.scripts, and if an entry matches no file it is dropped silently (no
warning, exit 0). The module is simply absent from the binary, and the only symptom is a
MODULE_NOT_FOUND crash the first time that code path runs, never a build failure. This is the
same class of silent-packaging gap that issues like DocumentServer#267 surfaced, and the same class of gap the Redis work in #44 and #46
runs into.

Each of the four components (DocService, FileConverter, Metrics, AdminPanel/server)
declares pkg.scripts entries, and a typo or a deleted/renamed source file would reintroduce the
same silent gap. This guard turns that class of silent omission into a loud build failure. It
complements a module-specific packaged smoke test like the one in #46: that proves one specific module is
bundled; this generic check protects every pkg.scripts entry in every component, at build time.

How it resolves globs (faithful to pkg)

@yao-pkg/pkg resolves each pkg.scripts entry as path.join(componentDir, entry) and then
globs it (via tinyglobby, {absolute:true, dot:true}), bundling a result only if it is a
file. Every current pkg.scripts entry is a literal path, and for a literal pattern that
resolution is equivalent to a plain fs.statSync(resolved).isFile() check. The guard therefore
uses that check and needs no new dependency. It also:

• treats a leading ! entry as a pkg exclusion (not a file requirement); and
• treats a bare @, + or ! inside a path as literal (so a scoped-package path such as
  node_modules/@scope/pkg/index.js is checked, not mistaken for a pattern), while refusing an
  entry that contains real glob syntax rather than guessing at it (none exist today).

It must run after npm install, since some entries point into node_modules (e.g. axios,
statsd) that only exist post-install, exactly as pkg requires. In both the production image
build and the e2e CI job it is wired after the component installs and before the pkg step.

No dependency choice

I considered using pkg's own matcher (tinyglobby) so the guard could resolve globs exactly as
pkg does. I did not, for two reasons: every current pkg.scripts entry is a literal path, for
which the built-in fs check is already identical to what pkg does; and a real glob is refused
rather than resolved, so no glob engine is needed. Staying dependency-free also means the guard
runs anywhere node is available, including build and CI steps that do not install the repo's
root dev-dependencies. If a glob is ever added to pkg.scripts, tinyglobby (pkg's own matcher)
is the documented upgrade path.

Discover components choice (instead of a static list)

The guard finds what to check by scanning for package.json files that declare a pkg.scripts
block, rather than hardcoding the list of components. A fixed list is one more thing to keep in
sync by hand, and its failure mode is quiet: add a component (or introduce pkg.scripts in an
existing one) and forget to update the list, and that component's entries silently escape the
guard, the very silent-omission this change exists to prevent. Discovery keeps coverage
automatic.

Possible objections

We could reconsider, going back to a static component list to address specific parts only, or even dropping this guard
altogether, if it turns out other developers deliberately rely on pkg's permissiveness (a missing
file dropping silently) to keep a module optional. But that is a very fragile mechanism: it is exactly
what produced the issues this PR addresses, which is why we want to catch the problem at build
time.

Changes

• tools/check-pkg-scripts.js: the guard (Node built-ins only, no new dependency).
• package.json: npm run check:pkg-scripts.
• .docker/server.bake.Dockerfile: run the guard before the pkg build (guards the real
  production packaging step).
• .github/workflows/e2e.yml: run the guard after install, before pkg (fast PR feedback).
• tests/unit/checkPkgScripts.tests.js + tests/fixtures/pkgScripts/*: regression tests
  (valid, zero-match, scoped-literal, glob-refusal, !-exclusion, non-string, no-scripts,
  missing package.json, and component discovery).

Try it

npm run check:pkg-scripts   # after installing the component deps

Timing to merge

The guard only passes once the files that pkg.scripts points at actually exist. On main today
a few entries reference a module that has not been added yet (for example the editorDataRedis
module that #46 introduces), so the guard will correctly fail and CI stays red until that module
lands. Two options for merge timing:

• Merge with or after the PR that adds the missing module (for example feat: add Redis-backed editor data storage for high availability #46 for editorDataRedis);
  the entries already exist, so the guard goes green once the file is there.
• Or remove the stale entries now and merge this PR green, in which case the PR that later adds the
  module must re-add its pkg.scripts entry (otherwise pkg would drop it again).

---

Hope this helps close this class of silent-omission gap..

Review and feedback appreciated,
thanks!

[MonaAghili]

I checked it against the real toolchain rather than only reading it: I built binaries with @yao-pkg/pkg 6.14.2 on a node20-linux-x64 target (that is what both builds actually get, see point 5) and ran them, with 6.23.0 for comparison. The core claim holds: an entry that matches no file is dropped with exit 0 and no warning, and the binary fails with MODULE_NOT_FOUND. Putting the guard in server.bake.Dockerfile also covers the .deb/.rpm packages, because DocumentServer's bundle target reuses the server target's binaries.

Here is what I found, most important first.

1. Merge timing: this breaks the DocumentServer builds, not only e2e

The guard runs in .docker/server.bake.Dockerfile. DocumentServer's CI runs one bake that includes packages and standalone, and both are built on bundle, which is built on server. DocumentServer's update-submodules.yml also opens one combined bump PR for all submodules every day (Euro-Office/DocumentServer#388 is open right now). If this PR lands while DocService/sources/editorDataRedis.js is still missing, that bump PR goes red for every submodule, and merging it anyway breaks the :nightly push on main.

So please don't merge this on its own while it is red. Either merge it with or right after #46, or drop the three stale editorDataRedis.js entries here (#46 then needs to re-add them).

2. A string-form pkg.scripts escapes the guard

pkg accepts "scripts": "./sources/x.js" (it wraps a non-array in an array). hasPkgScripts (tools/check-pkg-scripts.js:50) only accepts arrays, so such a component is never discovered and the guard prints "every entry resolves to a file". checkComponent (:103-108) only checks that scripts is truthy and then loops over it, so for a string it reports one failure per character (20 bogus failures for ./sources/missing.js).

Suggested fix: normalise the way pkg does, in both places, e.g. const list = Array.isArray(s) ? s : [s], and count any truthy pkg.scripts as a component.

3. A ! entry that overlaps a listed file drops that file

pkg passes all entries to a single globSync call, so a negation removes any positive entry it matches. With ["./sources/x.js", "!./sources/x.js"], pkg exits 0 and the binary fails with MODULE_NOT_FOUND, but the guard skips the ! entry (:114) and passes. Suggested fix: resolve literal negations the same way and fail when one equals a positive entry, and refuse negations that contain glob syntax, as positive entries already are.

4. Glob characters in the checkout path

The guard only checks the entry for glob syntax (:117), but pkg globs path.join(componentDir, entry), i.e. the whole absolute path. With the repo under a folder like a (b)/, pkg bundles nothing and the guard passes. CI (/home/runner/work/...) and Docker (/server) are not affected, so this only hits local builds, but it contradicts the "can never diverge from pkg" comment. Running GLOB_SYNTAX against the resolved path (or componentDir) and failing with a clear message would close it.

Repro for 2 to 4 (pkg 6.14.2, node20-linux-x64; the entry point does require('./sources/' + name) at runtime, and the binary is run from an unrelated folder):

scripts: "./sources/missing.js"                   pkg exit 0, not bundled   guard: component not discovered, exit 0
scripts: ["./sources/x.js", "!./sources/x.js"]    pkg exit 0, not bundled   guard: exit 0
repo under "a (b)/", ["./sources/x.js"]           pkg exit 0, not bundled   guard: exit 0
control: ["./sources/x.js"]                       pkg exit 0, bundled       guard: exit 0

5. pkg is not pinned, and the version you get depends on the builder's Node

Both builds run npm install -g @yao-pkg/pkg with no version (.docker/server.bake.Dockerfile:13, .github/workflows/e2e.yml:92). latest is 6.23.0, but 6.15.0 and later declare engines.node >= 22, so on the Node 20 builders npm resolves to 6.14.2 (today's e2e logs show pkg@6.14.2). Both versions resolve scripts with the same code today, so nothing diverges yet. But the guard describes itself as matching pkg's internals, while the pkg version will change silently the day the builders move to Node 22. I'd pin it (for example @yao-pkg/pkg@6.14.2) in both places and note in the guard's header comment which version its logic was checked against.

6. The error output is misleading for anything other than "matches no file"

The CLI always prints "the following entries match no file" and "@yao-pkg/pkg would drop these entries from the packaged binary silently" (:163-171). That is wrong for a glob refusal (real pkg bundles ./sources/*.js fine) and for a non-string entry (real pkg already fails with exit 2, "Config items must be strings"). A neutral header ("N problem(s) in pkg.scripts") and wording per failure reason would fix it.

7. Tests

• The glob-refusal test can't fail. With the refusal check deleted, the fallback reason is matches no file (globEntry/sources/*.js), and toMatch(/glob/i) (tests/unit/checkPkgScripts.tests.js:34) matches the fixture folder name globEntry. Asserting /not supported by this guard/ fixes it.
• I broke the guard in a few ways to see what the suite catches. It still passes when the node_modules/tests/.git pruning is removed, when symlinks are followed, and when the CLI exits 0 on failure. Only breaking the ! handling fails a test. The pruning and symlink cases probably need a temp directory built inside the test (fs.mkdtempSync), since a node_modules fixture folder won't get committed. The exit code can be checked with spawnSync against the same kind of temp repo.

8. Smaller points

• The comment at tools/check-pkg-scripts.js:44-46 says npm install parses every package.json first. It only parses the manifest of the folder being installed, and a malformed component manifest is silently left out of discovery. In practice pkg or that component's own install fails loudly, so only the comment is wrong.
• e2e checks AdminPanel/server but never installs it. That's fine today (its entries point at DocService sources), but a future node_modules entry there would make e2e fail while Docker passes.

Out of scope, but worth a follow-up issue

• pkg drops pkg.assets the same silent way (same expandFiles + isFile path). DocService's node_modules/sharp/build/Release/* and node_modules/sharp/vendor/**/* match today because sharp is pinned to 0.32.6. sharp 0.33+ loads from src/build/Release/ or @img/sharp-*, so both globs would match nothing after an upgrade. Covering assets needs a real glob engine (tinyglobby), which is a fair reason to revisit the no-dependency choice later.
• The guard catches stale entries, not the opposite case: a module loaded with a dynamic require that is missing from pkg.scripts. All dynamic requires are covered today (editorDataStorage/editorStatStorage in DocsCoServer.js, notificationService.js, routes/info.js and changes2forgotten.js), but "turns that class of silent omission into a loud build failure" in the description promises a bit more than the guard does.

Lint, Prettier and the 10 new tests pass, and the fixtures don't collide with anything in Jest.

[j-base64]

Thanks for the review!

On "1. Merge timing": this is the point I raised in the PR description under "Timing to merge". I don't have a single fixed answer and saw it as an open question: whether this PR should depend on the timing of the related PRs like #44 or #46.

a. Either we wait and land this with/after the one that adds the currently missing file,
or, b. we clean up and remove the problematic entries now (the editorDataRedis.js entries in package.json, from this PR or a small dedicated one) and ask the relevant PR author(s) (currently #44 or #46) to re-add them once the file exists on main.

I lean toward b via a small dedicated PR, as it could streamline getting this landed. Happy to discuss which way we prefer.

I will go through your other points and address them asap.

Thanks again!

[j-base64]

Pushed a revision that addresses the points you raised @MonaAghili and hardened the approach a bit further.
I've also left two open questions below that I'd rather discuss than settle on my own.

Cool if you can take a look 🙌.

---

✅ Fixed (your points):

• (2) string-form pkg.scripts: normalized the way pkg does (string -> one entry), in
  both discovery and the per-component check.
• (3) ! negation that cancels a listed file: a two-pass check records literal exclusions
  and fails the cancelled positive; glob negations are refused like positive globs.
• (4) glob metacharacters in the checkout path: detected against the resolved component
  path and reported once.
• (6) misleading output: neutral header ("N problem(s)"), the consequence now lives in
  each failure's own reason.
• (8) corrected the npm install comment and the e2e scope note (three binaries overlaid,
  AdminPanel checked but not overlaid).
  • On e2e not installing AdminPanel/server: harmless today, since its entries are DocService
    source paths, not its own node_modules, so I've kept it as documented rather than touch the
    CI here. Two fixes I considered and deliberately deferred for this latent case: install
    AdminPanel/server in e2e as well, or make the guard skip a node_modules entry when that
    component's deps are not installed in the current context. Happy to do one if you'd rather
    close it now, but neither felt worth the footprint for a case that can't trigger today.

💪 Solidified:

• (7) tests: the glob-refusal tautology is gone; failures are asserted via a stable kind
  code instead of message text, and the pruning / symlink / exit-code cases build a real
  temp repo (mkdtempSync) and drive the CLI with spawnSync, so they actually bite
  (verified by mutation).
• Refuse an entry that resolves to a directory (positive or negation): pkg expands a
  directory recursively, which this guard does not model, so it fails loud instead of
  guessing. New DIRECTORY_UNSUPPORTED kind + fixtures.
• Documented the assurance boundary in the header (checks pkg.scripts resolvability for
  discovered components; does not model the pkg invocation list or pkg.assets).

📌 Follow-up (agreeing with your steer):

• tinyglobby / pkg.assets: same silent-drop class as pkg.scripts. Worth keeping as follow-up issue
  to track it. Not pulling tinyglobby into pkg.scripts now, after weighing the pros/cons below.

  + why (pros/cons)

  • Every current pkg.scripts entry is a literal path, and for a literal
    fs.statSync(resolved).isFile() is identical to pkg's glob + isFile, so there is no
    resolution to buy today.
  • Neither run context installs the repo's root dev-deps (e2e installs the four component
    dirs, the Dockerfile installs each component then copies the tree) and pkg is installed
    globally, so a root tinyglobby dependency would not resolve where the guard runs.
    Adopting it now would mean adding a root install step to the production bake and e2e for
    zero entries that need a glob.
  • pkg.assets is the real case: those entries are globs (e.g. sharp), which statSync
    cannot resolve. When we cover assets, the clean move is to resolve pkg's own bundled
    tinyglobby from the global install rather than add a dependency, which also makes the
    guard faithful by construction instead of by comment.

---

❓ Two open questions:

1. Related to "1. Merge timing" point discussed above, about current stale editorDataRedis.js entries (entry present, file absent): three manifests list
   editorDataRedis.js, which isn't on main yet, so the guard correctly fails on them:

   • DocService/package.json → ❌ ./sources/editorDataRedis.js
   • FileConverter/package.json → ❌ ../DocService/sources/editorDataRedis.js
   • AdminPanel/server/package.json → ❌ ../../DocService/sources/editorDataRedis.js

   Two ways forward:

   • (a) Clean the room first: a separate tiny PR removes the three entries and this PR then merges green. Responsibility then shifts in a normal order:
     whichever PR lands the real file, like feat: add Redis-backed editor data storage for high availability #46 or Redis-backed save/auth locks and presence for editorData #44, must re-add and review all these three
     entries, FileConverter and AdminPanel included. Pro: isolated and easy to validate. Helps this PR land soon. Cons: require coordination with PRs Redis-backed save/auth locks and presence for editorData #44, feat: add Redis-backed editor data storage for high availability #46.
   • (b) Land together: merge this guard with or right after feat: add Redis-backed editor data storage for high availability #46; the entries resolve the
     moment the file exists, with no manifest changes anywhere. Cons: this PR stays open until feat: add Redis-backed editor data storage for high availability #46 adds the file on main, even though the guard is general and worth having now. Require coordination with PRs Redis-backed save/auth locks and presence for editorData #44, feat: add Redis-backed editor data storage for high availability #46 too.

   I prefer (a), but I'm open to your suggestions.

2. Pinning @yao-pkg/pkg: the install is unpinned today, which predates this PR. It is the one dep in the repo that is not pinned, so I lean toward fixing that repo-wide in its own PR rather than here. That said, do you think it is worth pinning it to 6.14.2 (the version CI resolves) in this PR too, so the guard carries a fixed reference for the resolution it models?

[MonaAghili]

Re-checked at f02e70e. Only the merge order is left from my side; answers to your two questions below.

1. Merge order: I'd go with (b), merge right after #46

I tried (a) and it fails silently. With the three editorDataRedis.js entries removed on main, #46 (at f62c462) merges with no conflict, because it never touches those lines. The result has DocService/sources/editorDataRedis.js on disk but no pkg.scripts entry for it in any of the three manifests.

The backend is only loaded through dynamic requires: Common/sources/notificationService.js:36 and DocService/sources/DocsCoServer.js (line 100 on main). Nothing requires editorDataRedis.js or editorDataRedis/index.js statically, so pkg doesn't bundle it. I built DocService from that tree with pkg 6.14.2 (node20-linux-x64) and ran it with editorDataStorage: "editorDataRedis". It dies at startup with Cannot find module './../../DocService/sources/editorDataRedis' (MODULE_NOT_FOUND, from notificationService.js via server.js). On the same tree, with deps installed, this guard prints "every entry resolves to a file" and exits 0, because it catches stale entries, not missing ones.

So (a) only works if someone remembers to re-add all three entries when the file lands, and nothing flags it if they don't. With (b), there's nothing to remember: merging this branch with #46 at f62c462 clears all three failures without touching any manifest, the guard passes, and the same binary starts normally in Redis mode.

2. Pinning pkg: fine in a separate PR

Not blocking for me. Pinning @yao-pkg/pkg here wouldn't fix the guard's reference anyway. It's a global install, and pkg ships no shrinkwrap, so the matcher resolves tinyglobby@^0.2.11 fresh on every build (0.2.17 today with pkg 6.14.2 on Node 20). The header already names the range it was checked against, which is the accurate statement either way.