fix(ci): install node-jq binary explicitly (npm 11 blocks its preinstall)#24
Merged
Conversation
The v0.19.0 release gate hung: every --jq test failed with 'spawn node_modules/node-jq/bin/jq ENOENT'. CI upgrades to npm@latest (needed for OIDC trusted publishing), and npm >= 11 blocks dependency lifecycle scripts by default, so node-jq's preinstall never downloaded its jq binary. Node 24's bundled npm 11 broke the CI matrix leg the same way. Run node-jq's installer directly (a plain node invocation, not an npm lifecycle hook) after npm ci in both workflows. Also corrects the testTimeout rationale: the real driver is the client retry tests' real exponential-backoff sleeps (~14s worst case), not jq; bump to 30s for safe headroom on coverage-instrumented CI. Claude-Session: https://claude.ai/code/session_018dRMUUj9vpKp3tqKQFsvE9
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
realCommandIds() split file paths on '/' only, so on Windows readdirSync's backslash paths produced ids like 'deal\list' that missed the audited map (4 failures on the windows-latest CI legs). Split on both separators. Claude-Session: https://claude.ai/code/session_018dRMUUj9vpKp3tqKQFsvE9
6155f2d to
c135d27
Compare
The 100% coverage gate failed on the windows-latest legs: makeExec's SIGTERM->SIGKILL grace escalation is unreachable there (Windows kills a child on the first signal), leaving its callback uncovered. Extract it as scheduleHardKill() and unit-test it directly with fake timers, so the escalation is exercised OS-independently while the real-process test keeps asserting end-to-end behavior on POSIX. Claude-Session: https://claude.ai/code/session_018dRMUUj9vpKp3tqKQFsvE9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
The v0.19.0 release gate hung: every
--jqtest timed out withspawn .../node_modules/node-jq/bin/jq ENOENT. CI runsnpm install -g npm@latest(required for OIDC trusted publishing), and npm >= 11 blocks dependency lifecycle scripts by default, so node-jq'spreinstallnever downloaded its jq binary. The CI matrix's Node-24 leg (bundled npm 11) failed identically. node-jq then hangs on the missing binary rather than erroring, so it surfaced as 20s timeouts, not a clear ENOENT.Not a code regression and not test contention — an npm policy change (v0.18 released before it).
Fix
Run node-jq's installer directly (
node node_modules/node-jq/scripts/install-binary.mjs, a plain node call, not an npm hook) afternpm ciin bothci.ymlandrelease.yml. Also corrected thetestTimeoutcomment/value: the real driver is the client retry tests' real exponential-backoff sleeps (~14s worst case), bumped to 30s.Gate before merge: this PR's CI matrix must go green.
Follow-up (not here)
npm-install end users on npm 11 also get a broken/hanging
--jq. v0.20 (distribution) will add a runtime guard so a missing jq binary fails fast with a clear message instead of hanging.https://claude.ai/code/session_018dRMUUj9vpKp3tqKQFsvE9
Greptile Summary
This PR fixes a CI breakage introduced by npm ≥ 11 blocking dependency lifecycle scripts, which prevented
node-jq'spreinstallhook from downloading thejqbinary, causing all--jqtests to hang withENOENT. Both workflow files now invokenode-jq's installer directly afternpm ci.node node_modules/node-jq/scripts/install-binary.mjsas an explicit step; the script path is confirmed present innode-jq≥ 6.x.invoke.js: extractsscheduleHardKillso the SIGKILL escalation is unit-testable on Windows; a new fake-timer test covers it.catalog.test.js/vitest.config.js: fixes Windows path-separator normalisation and corrects thetestTimeoutcomment/value to reflect actual retry-backoff sleep budgets.Confidence Score: 5/5
Safe to merge — changes are narrowly scoped to CI plumbing, a small testability refactor, and two correctness fixes in tests.
All changes are well-targeted: the workflow fix uses the confirmed-valid scripts/install-binary.mjs path from node-jq's published package, the scheduleHardKill extraction is a non-functional refactor backed by a new deterministic test, the Windows path-separator fix is correct, and the timeout bump matches the stated worst-case retry sleep budget.
No files require special attention.
Important Files Changed
Reviews (3): Last reviewed commit: "test(mcp): make the SIGKILL escalation c..." | Re-trigger Greptile