build: use node to detect package.json#pnpm in update-js

[pedrogaudencio]

• replace indentation-anchored grep guard with a JSON.parse-based check
• guard now holds regardless of package.json formatting or nested engines.pnpm

Closes #306

AI Disclosure

Co-authored with: Opus 5

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[pedrogaudencio]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

I reviewed the diff, verified the guard's exit-code semantics empirically (with the repo's real package.json, a 4-space-indented pnpm key, and an engines-only file), checked NODE_VARS/node-check interactions, and searched for other places the old grep guard or nolyfill logic might be duplicated. The change is functionally correct: it fires on any top-level pnpm key regardless of formatting, passes on the repo's actual package.json (which only has nested engines.pnpm at package.json:5), and node availability is guaranteed by the node-check prerequisite (update-js: node-check | node_modules). Omitting $(NODE_VARS) is also fine, since that variable only enables TS type-stripping, which plain -e JavaScript doesn't need.

One improvement found:

1. 🟡 The JSON check can be simplified — require() already parses JSON in node -e

• Addressed
• Dismissed

Makefile:889 is a dense 110+-character one-liner that manually reads and parses the file:

	@if node -e 'process.exit(JSON.parse(require("node:fs").readFileSync("package.json", "utf8")).pnpm === undefined ? 1 : 0)'; then \

In CommonJS node -e, require() resolves .json paths relative to the current working directory and parses them automatically, so the whole read/parse dance can be dropped. I verified both variants produce identical exit codes for the three relevant cases (no pnpm key, nested engines.pnpm, top-level pnpm at arbitrary indentation):

	@if node -e 'process.exit(require("./package.json").pnpm === undefined ? 1 : 0)'; then \

If you also want the check to state its intent ("does a top-level pnpm key exist?", matching the comment above it) rather than comparing against undefined, the in operator works too:

	@if node -e 'process.exit("pnpm" in require("./package.json") ? 0 : 1)'; then \

(For JSON.parse output the two forms are equivalent, since JSON cannot encode an undefined value; the require variant additionally tolerates a BOM, which readFileSync + JSON.parse would choke on.) Purely a readability change — the current implementation is functionally sound.

No other issues: no spelling/grammar problems in the echo messages, no naming concerns, no duplicated guards elsewhere in the Makefile or CI workflows, and no code made stale by the change.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[pedrogaudencio]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

Review: build: use node to detect package.json#pnpm in update-js

I verified the guard's exit-code logic empirically against the repo's real package.json (nested engines.pnpm, no top-level pnpm → passes), a 4-space-indented file with top-level pnpm (→ fires, fixing #306), and a malformed file. The behavior is correct in the cases that matter; the issues below are about robustness and readability.

1. 🟡 Double-negated exit logic is a trap for future editors

• Addressed
• Dismissed

Makefile:889:

	@if node -e 'process.exit(JSON.parse(require("node:fs").readFileSync("package.json", "utf8")).pnpm === undefined ? 1 : 0)'; then \

The shell if takes the error branch on exit code 0, but the JS expresses the absence of the key (=== undefined) and maps it to 1. A reader (or a future edit) has to invert the condition twice to confirm the guard fires in the right direction — exactly the kind of line that gets "fixed" into the wrong polarity later. Phrase it positively so exit 0 reads as "problem found":

	@if node -e 'process.exit(JSON.parse(require("node:fs").readFileSync("package.json", "utf8")).pnpm !== undefined ? 0 : 1)'; then \

If you also want it shorter, require resolves ./ against the cwd and parses JSON automatically (verified equivalent for both cases, including the 4-space-indented one):

	@if node -e 'process.exit(require("./package.json").pnpm !== undefined ? 0 : 1)'; then \

2. ⚪️ Guard fails open if package.json cannot be parsed

• Addressed
• Dismissed

Same line, Makefile:889. If JSON.parse throws (truncated/corrupted write, BOM, stray trailing text), node exits 1 with a stack trace on stderr, the shell if treats that as "clean", and the recipe proceeds to pnpm install — the silent-override-loss scenario this guard exists to prevent. Confirmed empirically: malformed JSON → exit=1 → error branch skipped. Realistically the earlier pnpm exec updates / nolyfill install steps would fail first on a broken file, hence the low rating, but a guard whose purpose is "never fail silently" should fail closed:

	@if node -e 'try { process.exit(JSON.parse(require("node:fs").readFileSync("package.json", "utf8")).pnpm !== undefined ? 0 : 1) } catch { console.error("package.json is not parseable JSON; cannot verify pnpm overrides."); process.exit(0) }'; then \

(Optional catch binding supported since the repo requires Node 22.)

3. ⚪️ Same grep-fragility remains in the adjacent node-check (out-of-diff follow-up)

• Addressed
• Dismissed

Makefile:227:

	$(eval MIN_NODE_VERSION_STR := $(shell grep -Eo '"node":.*[0-9.]+"' package.json | sed -n 's/.*[^0-9.]\([0-9.]*\)"/\1/p'))

This is the exact class of formatting-dependent text scraping this PR eliminates — it grabs the first "node": occurrence anywhere in the file, so any nested "node" key appearing above engines (or a reformatted file) can silently yield the wrong version. Not part of this diff, but since the fix pattern is now established here, a follow-up applying the same node -e/JSON.parse treatment would be consistent, e.g.:

	$(eval MIN_NODE_VERSION_STR := $(shell node -p 'JSON.parse(require("node:fs").readFileSync("package.json", "utf8")).engines.node'))

No other stale references to the old grep exist (checked Makefile, workflows, docs), and the changed line needs no $(NODE_VARS) prefix since the script is plain JS with no $/# characters for make or the shell to mangle.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[pedrogaudencio]

@taoeffect approved! ✅