## Summary#192
Conversation
The PR Validation "Build, typecheck, and test" check fails on every dependency PR at the audit step. npm 10 (bundled with Node 20) crashes on the aliased "@posthog/cli": "npm:empty-npm-package@1.0.0" override with "Invalid comparator", so `npm audit` exits non-zero before evaluating any advisory and `set -o pipefail` fails the job. This is independent of what each PR bumps, which is why even trivial patch PRs go red. - Run audit with npm 11 (handles aliased overrides) via npx. - Evaluate results with .github/scripts/check-audit.mjs, which fails on high/critical advisories except a documented allowlist, and fails loudly if audit itself cannot run (never silently skips the gate). - Allowlist the samlify advisory (GHSA-34r5-q4jw-r36m): it surfaces transitively via @better-auth/sso, which pins samlify ~2.10.2, so the patched samlify >=2.13.0 would risk breaking SAML/SSO. No safe upstream fix exists yet; re-evaluate when @better-auth/sso bumps samlify. No production dependencies change; package.json/package-lock.json are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds an npm audit allowlist gate to the CI pipeline. A new Node.js script validates ChangesAudit allowlist enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the reqcore-pr-192 environment in applirank
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/pr-validation.yml (1)
55-55: ⚡ Quick win
audit-error.txtis captured but never surfaced.npm's stderr is redirected to
audit-error.txt, but nothing reads it (the summary only catsaudit-output.txt). If npm 11 audit fails to emit valid JSON,check-audit.mjsexits 2 with a generic "Could not read/parse" message while the real npm error stays hidden — losing diagnostics for the exact crash class this PR addresses. Consider echoing it on non-zero parse, or including it in the step summary.🔭 Surface npm stderr when the audit JSON can't be evaluated
npx -y npm@11 audit --json > audit-output.json 2> audit-error.txt || true - node .github/scripts/check-audit.mjs audit-output.json 2>&1 | tee audit-output.txt + node .github/scripts/check-audit.mjs audit-output.json 2>&1 | tee audit-output.txt + rc=${PIPESTATUS[0]} + if [ "$rc" -eq 2 ] && [ -s audit-error.txt ]; then + echo "--- npm audit stderr ---" | tee -a audit-output.txt + cat audit-error.txt | tee -a audit-output.txt + fi + exit "$rc"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-validation.yml at line 55, The workflow redirects npm stderr to audit-error.txt but never surfaces it; update the step and/or the check-audit.mjs error path to include that stderr when JSON parsing fails: keep the existing command (npx -y npm@11 audit --json > audit-output.json 2> audit-error.txt || true) but modify the failure handling in check-audit.mjs (or the workflow step that calls it) so that on non-zero/parse errors it reads audit-error.txt and prints or adds its contents to the step summary/console output (include the filename audit-error.txt and a clear message), ensuring the parse failure path logs the exact npm stderr for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/check-audit.mjs:
- Around line 61-67: The loop currently ignores high/critical advisories that
lack via.url; change it so all high/critical via entries are recorded into the
advisories Map even when via.url is falsy: in the block iterating vuln.via
(symbols: advisories, via, vuln, vuln.via) remove the guard that skips when
via.url is falsy and instead compute a stable key (e.g. via.url ||
`${vuln.name||vuln.title||'advisory'}:${via.severity}:${via.id||i}`) and call
advisories.set(key, { url: via.url || null, title: via.title, severity:
via.severity, id: via.id || null }) so entries without a url are captured (with
url null) and will block by default and can be matched by an allowlist
separately.
In @.github/workflows/pr-validation.yml:
- Around line 55-56: Pin the floating npm invocation and surface the stderr file
so audit failures are reproducible and visible: change the npx call (currently
"npx -y npm@11 audit ...") to a pinned release such as "npx -y npm@11.16.0 audit
--json > audit-output.json 2> audit-error.txt || true" and update the downstream
invocation of the audit checker (node .github/scripts/check-audit.mjs) to also
consume or print audit-error.txt (for example pass audit-error.txt as an
additional argument to check-audit.mjs or cat audit-error.txt into the piped
output) so the contents of audit-error.txt are included in audit-output.txt and
the PR summary; ensure references to "npx -y npm@11", "audit-error.txt", and
"node .github/scripts/check-audit.mjs" are updated accordingly.
---
Nitpick comments:
In @.github/workflows/pr-validation.yml:
- Line 55: The workflow redirects npm stderr to audit-error.txt but never
surfaces it; update the step and/or the check-audit.mjs error path to include
that stderr when JSON parsing fails: keep the existing command (npx -y npm@11
audit --json > audit-output.json 2> audit-error.txt || true) but modify the
failure handling in check-audit.mjs (or the workflow step that calls it) so that
on non-zero/parse errors it reads audit-error.txt and prints or adds its
contents to the step summary/console output (include the filename
audit-error.txt and a clear message), ensuring the parse failure path logs the
exact npm stderr for debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e09b360c-47e3-4576-af56-3d410c62968f
📒 Files selected for processing (2)
.github/scripts/check-audit.mjs.github/workflows/pr-validation.yml
| for (const vuln of Object.values(vulns)) { | ||
| for (const via of vuln.via || []) { | ||
| if (typeof via !== "object") continue; | ||
| if (via.severity !== "high" && via.severity !== "critical") continue; | ||
| if (via.url) advisories.set(via.url, { url: via.url, title: via.title, severity: via.severity }); | ||
| } | ||
| } |
There was a problem hiding this comment.
High/critical advisories without a url are silently dropped from the gate.
A via advisory object is only recorded when via.url is truthy (Line 65). Any high/critical advisory object lacking a url is skipped entirely, so it neither blocks nor gets reported. It also can't be allowlisted (the allowlist keys on url), so the safe default for such an entry is to block. In practice npm/GHSA advisories carry a url, hence the low priority, but for a security gate it's worth failing closed.
🛡️ Fail closed when a high/critical advisory has no url
for (const vuln of Object.values(vulns)) {
for (const via of vuln.via || []) {
if (typeof via !== "object") continue;
if (via.severity !== "high" && via.severity !== "critical") continue;
- if (via.url) advisories.set(via.url, { url: via.url, title: via.title, severity: via.severity });
+ // Use the url as the dedup/allowlist key; fall back to a synthetic key so
+ // an advisory without a url still surfaces and blocks (it can't be allowlisted).
+ const key = via.url || `${via.source ?? via.title ?? "unknown"}`;
+ advisories.set(key, { url: via.url || "(no url)", title: via.title, severity: via.severity });
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const vuln of Object.values(vulns)) { | |
| for (const via of vuln.via || []) { | |
| if (typeof via !== "object") continue; | |
| if (via.severity !== "high" && via.severity !== "critical") continue; | |
| if (via.url) advisories.set(via.url, { url: via.url, title: via.title, severity: via.severity }); | |
| } | |
| } | |
| for (const vuln of Object.values(vulns)) { | |
| for (const via of vuln.via || []) { | |
| if (typeof via !== "object") continue; | |
| if (via.severity !== "high" && via.severity !== "critical") continue; | |
| // Use the url as the dedup/allowlist key; fall back to a synthetic key so | |
| // an advisory without a url still surfaces and blocks (it can't be allowlisted). | |
| const key = via.url || `${via.source ?? via.title ?? "unknown"}`; | |
| advisories.set(key, { url: via.url || "(no url)", title: via.title, severity: via.severity }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/check-audit.mjs around lines 61 - 67, The loop currently
ignores high/critical advisories that lack via.url; change it so all
high/critical via entries are recorded into the advisories Map even when via.url
is falsy: in the block iterating vuln.via (symbols: advisories, via, vuln,
vuln.via) remove the guard that skips when via.url is falsy and instead compute
a stable key (e.g. via.url ||
`${vuln.name||vuln.title||'advisory'}:${via.severity}:${via.id||i}`) and call
advisories.set(key, { url: via.url || null, title: via.title, severity:
via.severity, id: via.id || null }) so entries without a url are captured (with
url null) and will block by default and can be matched by an allowlist
separately.
| npx -y npm@11 audit --json > audit-output.json 2> audit-error.txt || true | ||
| node .github/scripts/check-audit.mjs audit-output.json 2>&1 | tee audit-output.txt |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest npm 11 version and its required Node.js engines range?
💡 Result:
As of May 29, 2026, the latest version of npm 11 is 11.16.0 [1]. The required Node.js engines range for npm 11 is defined as ^20.17.0 || >=22.9.0 [2]. This range was established to support newer Node.js releases while ensuring compatibility with supported LTS versions [2].
Citations:
- 1: https://docs.npmjs.com/cli/v11/using-npm/changelog/
- 2: sets node engine range to
^20.17.0 || >=22.9.0npm/cli#7831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the workflow section around the referenced lines and any setup-node config nearby.
FILE=".github/workflows/pr-validation.yml"
if [ -f "$FILE" ]; then
echo "== $FILE =="
nl -ba "$FILE" | sed -n '1,140p'
else
echo "Missing $FILE" >&2
exit 1
fi
# Also show the check-audit script for context on what error outputs it consumes.
SCRIPT=".github/scripts/check-audit.mjs"
if [ -f "$SCRIPT" ]; then
echo "== $SCRIPT =="
nl -ba "$SCRIPT" | sed -n '1,200p'
else
echo "Missing $SCRIPT" >&2
exit 1
fiRepository: reqcore-inc/reqcore
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/pr-validation.yml"
if [ -f "$FILE" ]; then
echo "== $FILE =="
awk 'NR>=1 && NR<=160 {printf "%d:%s\n", NR, $0}' "$FILE"
else
echo "Missing $FILE" >&2
exit 1
fi
echo
echo "== setup-node occurrences =="
rg -n "setup-node|node-version" .github/workflows/pr-validation.yml .github/workflows -S || true
echo
SCRIPT=".github/scripts/check-audit.mjs"
if [ -f "$SCRIPT" ]; then
echo "== $SCRIPT =="
awk 'NR>=1 && NR<=220 {printf "%d:%s\n", NR, $0}' "$SCRIPT"
else
echo "Missing $SCRIPT" >&2
exit 1
fiRepository: reqcore-inc/reqcore
Length of output: 8324
Pin npm@11 (and surface audit-error.txt)—Node 20 is compatible
actions/setup-node@v6usesnode-version: 20; npm 11’s engines are^20.17.0 || >=22.9.0, so the current Node 20 setup should satisfy the requirement.audit-error.txtis still never surfaced in the PR summary; ifnpm auditfails before producing useful JSON, the stderr context won’t be visible (onlyaudit-output.txtis).npx -y npm@11is floating; pinning to a specific npm 11.x version (e.g.,npm@11.16.0) would make the audit gate reproducible over time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-validation.yml around lines 55 - 56, Pin the floating
npm invocation and surface the stderr file so audit failures are reproducible
and visible: change the npx call (currently "npx -y npm@11 audit ...") to a
pinned release such as "npx -y npm@11.16.0 audit --json > audit-output.json 2>
audit-error.txt || true" and update the downstream invocation of the audit
checker (node .github/scripts/check-audit.mjs) to also consume or print
audit-error.txt (for example pass audit-error.txt as an additional argument to
check-audit.mjs or cat audit-error.txt into the piped output) so the contents of
audit-error.txt are included in audit-output.txt and the PR summary; ensure
references to "npx -y npm@11", "audit-error.txt", and "node
.github/scripts/check-audit.mjs" are updated accordingly.
1 participant
This PR updates the CI configuration to run
npm auditwith npm 11, addressing issues with aliased overrides that caused failures in the audit step.This change is necessary to ensure that the audit process can handle aliased packages without crashing, allowing for proper evaluation of vulnerabilities.
Type of change
Bug fix
Feature
Refactor
Docs
Chore
Validation
I tested locally
I added/updated relevant documentation
I verified multi-tenant scoping and auth behavior for affected API paths
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit