Repository navigation
ci: vitest 4 to clear npm audit, Node 18.18 moves to a runtime smoke job - #31
Conversation
`npm audit` in plugins/cursor reported 15 advisories (3 critical, 6 high, 6 moderate), all in dev dependencies: vitest 2.1.9 and @vitest/coverage-v8 with tinypool, @vitest/mocker, vite, esbuild, glob and minimatch below them, plus postcss, nanoid, source-map-js, js-yaml and brace-expansion. Users are not affected: the plugin ships with zero dependencies and nobody runs `npm install` in the plugin cache. No vitest release without known advisories runs on Node 18 (2.x and 3.x depend on tinypool ^1; 4.x fails at startup because rolldown imports `styleText` from `node:util`). So: - vitest and @vitest/coverage-v8 go to ^4.1.12; the rest is a lockfile update. `npm audit` reports 0 vulnerabilities. - The test matrix runs on Node 20 and 22. `engines.node` stays >=18.18, because that describes the shipped scripts, which have no dependencies. - A new `node-18` job keeps that floor checked without dev dependencies: on Node 18.18 it parses and loads every script and runs `scripts/status.mjs`. - vitest 4 remaps V8 coverage through the AST, so statements are no longer counted as lines and the same suite reads 79.7 % statements against the 80 % threshold of `npm run test:coverage`. The thresholds stay; tests for `invokedAsScript` and for the rejection paths of the structured review output bring it to 80.7 %. - AGENTS.md, CONTRIBUTING.md and the README describe the CI as it is now. No runtime file, plugin.json or version changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
freema
left a comment
There was a problem hiding this comment.
CodeForge Review
Verdict: approve | Score: 9/10
Clean, well-scoped maintenance PR: bumps vitest 2→4 (clearing an npm audit advisory), narrows the main CI matrix to Node 20/22 since vitest 4 requires it, adds a lightweight Node 18.18 'runtime smoke' job (syntax-check + import every script + run one command) to keep verifying the zero-dependency plugin still works on the minimum supported Node version, and updates AGENTS.md/CONTRIBUTING.md/README.md consistently to describe the new CI shape. Also adds solid new unit tests for invokedAsScript (symlink handling, missing entry point, REPL case) and additional malformed-input coverage for parseReviewOutput. The package-lock.json changes are consistent with the vitest major bump and all resolved packages point to the official npm registry — nothing suspicious. No logic in the shipped plugin scripts is touched.
Reviewed by CodeForge
| run: | ||
| working-directory: plugins/cursor | ||
|
|
||
| steps: |
There was a problem hiding this comment.
[MINOR] The for f in scripts/*.mjs scripts/lib/*.mjs loop relies on bash glob expansion; if either directory ever had no matching .mjs files, the unexpanded literal pattern would be passed to node --check and fail with a confusing 'file not found' error rather than a clear message about the glob not matching.
Suggestion: Optional:
shopt -s nullglobbefore the loop, or add an explicit count check, so a future empty directory fails with a clearer message. Not a blocker since both directories currently contain files.
|
|
||
| it('rejects a missing or blank summary', () => { | ||
| // `undefined` is dropped by JSON.stringify, so the key is absent. | ||
| for (const summary of [undefined, ' ']) { |
There was a problem hiding this comment.
[SUGGESTION] The new 'rejects a missing or blank summary' test uses { ...GOOD, summary } with summary: undefined, relying on the fact that JSON.stringify drops undefined properties (and the comment calls this out) — correct but a bit implicit/fragile if withBlock ever changes its serialization approach.
Suggestion: No action needed; the inline comment already documents the subtlety, which is sufficient.
Summary
npm auditinplugins/cursorreported 15 advisories (3 critical, 6 high, 6 moderate), all in dev dependencies:vitest2.1.9 and@vitest/coverage-v8withtinypool,@vitest/mocker,vite,esbuild,globandminimatchbelow them, pluspostcss,nanoid,source-map-js,js-yamlandbrace-expansion. After this PR it reports 0.Plugin users are not affected either way: the plugin ships with zero dependencies and nobody runs
npm installin the plugin cache. No runtime file,plugin.jsonor version changes here.The part that needs a decision: Node 18.18 in CI
No vitest release without known advisories runs on Node 18. vitest 2.x and 3.x depend on
tinypool ^1(critical, no backport), and vitest 4 fails at startup on 18.18 becauserolldownimportsstyleTextfromnode:util. A clean audit and "the suite runs on Node 18.18" cannot both hold, so this PR does the following:vitestand@vitest/coverage-v8go to^4.1.12(the smallest step that clears the advisories; vitest 5 would also drop Node 20). Everything else is a lockfile update.engines.nodestays>=18.18. It describes the shipped scripts, which have no dependencies.node-18job keeps that floor checked without any dev dependencies: on Node 18.18 it parses every script (node --check), loads every script (which resolves all theirnode:imports), and runsscripts/status.mjsend to end. This is weaker than the full suite, but it catches a script that starts using an API newer than 18.18 at load time.AGENTS.md,CONTRIBUTING.mdand the README (FAQ and Contributing) now describe the CI as it is.If dropping Node 18 from
enginesis preferred instead (it has been end-of-life since April 2025), thenode-18job can simply be removed in the release that raises the floor.Coverage
vitest 4 remaps V8 coverage through the AST, so statements are no longer counted as lines. The unchanged suite then reads 79.7 % statements against the 80 % threshold of
npm run test:coverage(CI does not run it). The thresholds stay as they are. Two small additions bring statements to 80.7 %:tests/invoked.test.mjs:invokedAsScriptfor the module itself, through a symlink, for another entry point, with no entry point, and with a missing one.tests/review-output.test.mjs: non-object top level, missing or blank summary, and each way a single finding can be malformed.Test plan
npm test— all specs green (20 files, 188 passed, 4 skipped) on Node 20.19 and 22.21npm run lint— prettier + eslint cleannpm run typecheckclean;npm run test:coveragepasses its thresholdsnpm audit: 0 vulnerabilities;npm ciworks with npm 10 (Node 20 and 22)node-18steps run innode:18.18-alpine(28 modules load on 18.18.2,status.mjsexits 0)README.md,AGENTS.md,CONTRIBUTING.md) when relevant; noCHANGELOG.mdentry because nothing user-visible changesChecklist
AGENTS.md)dist/or.tsfiles$ARGUMENTSis quoted in any new / editedcommands/*.md(none touched)CONTRIBUTING.md(none)## UnreleasedinCHANGELOG.md(none)Note on the lockfile: npm 10 crashes while resolving this bump (
Cannot read properties of null (reading 'edgesOut')), so the tree was resolved with npm 11 and completed with npm 10.npm ciand a no-opnpm installsucceed on npm 10 with the committed file.🤖 Generated with Claude Code