Added check to fail if NPM commands are used#49
Conversation
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughA new GitHub Actions workflow file has been added that automatically detects npm command usage within other workflow files. When triggered, the workflow scans all YAML files in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai full review and reevaluate all your previous comments that may have been dismissed, ignored or resolved. Evaluate whether the goals of the issue linked in the first PR comment are met. If there are test cases, ensure that the updated or created tests won't be flaky, especially in a sharded testing environment where test cleanup is important and response times are highly variable. Post your conclusion as an easily readable comment that also includes your findings outside the diffs. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5007 🏁 Script executed: Length of output: 55932
📋 Executive SummaryGoal Achievement: The PR adds a reusable workflow to detect npm commands, but has several implementation issues and is not integrated into any actual workflow, meaning it won't run automatically. 🔍 Detailed Findings✅ What Works
❌ Critical Issues1. Not Integrated - Won't Actually RunThe workflow uses
2. Narrow Detection PatternThe current pattern # These would NOT be detected:
npm install # (has tab instead of space)
npm; # (semicolon)
npm|grep # (pipe)
run: npm # (line ending)
npm\ninstall # (newline)Recommendation: Use a more robust pattern: grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY"3. False PositivesWould incorrectly flag legitimate occurrences: # This comment says: Don't use npm install
# ^ Would fail the check despite being helpful documentation
- name: Error handling
run: echo "Error: npm command not found"
# ^ Would fail despite being just an error messageRecommendation: Add context-aware detection or document acceptable patterns 4. Limited ScopeOnly scans
Recommendation: Either expand scope or clearly document limitations 5. No Tests
Recommendation: Add test cases with: # Test files with npm commands (should fail)
# Test files without npm commands (should pass)
# Test files with "npm" in comments (expected behavior?)📍 Missing InformationNo Linked Issue: The PR body doesn't link to an issue. This makes it difficult to:
🎯 Recommendations for Fix
💡 Suggested ImplementationHere's an improved detection script: FORBIDDEN_COMMAND="npm"
WORKFLOW_DIRECTORY=".github/workflows"
# Use word boundaries to match npm as a command, not in words like "pnpm"
# Exclude comment lines and this workflow file itself
if grep -rE --exclude="$(basename "$0")" '^[^#]*\bnpm\b' "$WORKFLOW_DIRECTORY"; then
echo "::error::Forbidden 'npm' command found in workflow files!"
echo "Please use 'pnpm' instead."
echo "Matches found:"
grep -rnE --exclude="$(basename "$0")" '^[^#]*\bnpm\b' "$WORKFLOW_DIRECTORY"
exit 1
else
echo "✅ No forbidden 'npm' commands found."
fi📊 ConclusionWhile the PR introduces the mechanism for npm detection, it does not fully achieve the stated goal because:
Suggested Next Steps:
Would you like me to help implement these improvements? |
![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/pull-request-no-npm.yml (1)
15-17: Scan scope is limited to.github/workflows/.npm commands can appear in shell scripts,
Makefiletargets,package.jsonscripts, CI configuration files in other directories, or documentation. Restricting the scan to.github/workflows/misses all of those.♻️ Broaden scan scope to the entire repository
- WORKFLOW_DIRECTORY=".github/workflows" + SCAN_ROOT="." - if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY" --exclude="$SELF_WORKFLOW"; then + if grep -rE '\bnpm\b' "$SCAN_ROOT" \ + --include="*.yml" --include="*.yaml" \ + --include="*.sh" --include="Makefile" \ + --exclude-dir=".git" \ + --exclude-dir="node_modules" \ + --exclude="pull-request-no-npm.yml"; thenIf the intent is strictly to police workflow files only, document that limitation explicitly so future maintainers understand it is a deliberate scope restriction rather than an oversight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pull-request-no-npm.yml around lines 15 - 17, The grep scope is limited by the WORKFLOW_DIRECTORY variable and the conditional using grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY", which misses npm occurrences elsewhere; update the check so it scans the entire repository (e.g., run grep recursively from the repo root instead of "$WORKFLOW_DIRECTORY" or include other common locations like scripts/, Makefile, and package.json) or, if scanning only workflows is intentional, add an explicit comment/variable (and documentation) stating the deliberate limitation so maintainers understand the narrow scope; modify the WORKFLOW_DIRECTORY usage and the grep invocation accordingly and document the chosen scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pull-request-no-npm.yml:
- Around line 3-4: The workflow currently uses workflow_call so it is never run
on PRs; either add a pull_request trigger to this file (replace or augment on:
workflow_call with on: pull_request) so the workflow runs automatically for pull
requests, or create a separate caller workflow that uses this reusable workflow
(invoking the file that contains workflow_call) and triggers on pull_request;
update the file to reference pull_request where appropriate or create a new
.github/workflows/* workflow that uses the reusable workflow.
- Around line 13-23: The self-scan currently fails because the grep line (grep
-rE '\bnpm\b' "$WORKFLOW_DIRECTORY") scans this workflow file itself; update the
check in the block that sets WORKFLOW_DIRECTORY and runs grep so it excludes the
current workflow file (use the GITHUB_WORKFLOW_REF env var or hardcode the
filename) — e.g., modify the grep/find invocation used in the detection step to
skip "$GITHUB_WORKFLOW_REF" (or ".github/workflows/pull-request-no-npm.yml") so
the workflow file is not scanned and the error/exit logic only triggers for
other workflow files.
---
Nitpick comments:
In @.github/workflows/pull-request-no-npm.yml:
- Around line 15-17: The grep scope is limited by the WORKFLOW_DIRECTORY
variable and the conditional using grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY",
which misses npm occurrences elsewhere; update the check so it scans the entire
repository (e.g., run grep recursively from the repo root instead of
"$WORKFLOW_DIRECTORY" or include other common locations like scripts/, Makefile,
and package.json) or, if scanning only workflows is intentional, add an explicit
comment/variable (and documentation) stating the deliberate limitation so
maintainers understand the narrow scope; modify the WORKFLOW_DIRECTORY usage and
the grep invocation accordingly and document the chosen scope.
| - name: Scan workflow file for forbidden npm commands | ||
| run: | | ||
| WORKFLOW_DIRECTORY=".github/workflows" | ||
|
|
||
| if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY"; then | ||
| echo "::error::Forbidden 'npm' command found in one or more workflow files!" | ||
| echo "Please use 'pnpm' instead." | ||
| exit 1 | ||
| else | ||
| echo "No forbidden 'npm' commands found. The job can proceed." | ||
| fi |
There was a problem hiding this comment.
Self-scan causes the workflow to always fail — the detection is broken.
grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY" scans every file under .github/workflows/, which includes this workflow file itself. The file contains the word "npm" as a standalone token in at least three places (the name: fields on lines 1 and 13, and the error message on line 18), so the check immediately matches and exits with code 1 on every run regardless of actual violations.
The fix is to exclude the current workflow file from the scan. The cleanest approach uses GITHUB_WORKFLOW_REF:
🐛 Proposed fix — exclude the current workflow file from the scan
- name: Scan workflow file for forbidden npm commands
run: |
WORKFLOW_DIRECTORY=".github/workflows"
+ SELF_WORKFLOW=$(basename "$GITHUB_WORKFLOW_REF" | cut -d'@' -f1)
- if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY"; then
+ if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY" --exclude="$SELF_WORKFLOW"; then
echo "::error::Forbidden 'npm' command found in one or more workflow files!"
echo "Please use 'pnpm' instead."
exit 1
else
echo "No forbidden 'npm' commands found. The job can proceed."
fiIf you prefer a simpler, hardcoded exclusion (acceptable since the filename is stable):
- if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY"; then
+ if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY" --exclude="pull-request-no-npm.yml"; then📝 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.
| - name: Scan workflow file for forbidden npm commands | |
| run: | | |
| WORKFLOW_DIRECTORY=".github/workflows" | |
| if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY"; then | |
| echo "::error::Forbidden 'npm' command found in one or more workflow files!" | |
| echo "Please use 'pnpm' instead." | |
| exit 1 | |
| else | |
| echo "No forbidden 'npm' commands found. The job can proceed." | |
| fi | |
| - name: Scan workflow file for forbidden npm commands | |
| run: | | |
| WORKFLOW_DIRECTORY=".github/workflows" | |
| SELF_WORKFLOW=$(basename "$GITHUB_WORKFLOW_REF" | cut -d'@' -f1) | |
| if grep -rE '\bnpm\b' "$WORKFLOW_DIRECTORY" --exclude="$SELF_WORKFLOW"; then | |
| echo "::error::Forbidden 'npm' command found in one or more workflow files!" | |
| echo "Please use 'pnpm' instead." | |
| exit 1 | |
| else | |
| echo "No forbidden 'npm' commands found. The job can proceed." | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pull-request-no-npm.yml around lines 13 - 23, The
self-scan currently fails because the grep line (grep -rE '\bnpm\b'
"$WORKFLOW_DIRECTORY") scans this workflow file itself; update the check in the
block that sets WORKFLOW_DIRECTORY and runs grep so it excludes the current
workflow file (use the GITHUB_WORKFLOW_REF env var or hardcode the filename) —
e.g., modify the grep/find invocation used in the detection step to skip
"$GITHUB_WORKFLOW_REF" (or ".github/workflows/pull-request-no-npm.yml") so the
workflow file is not scanned and the error/exit logic only triggers for other
workflow files.
|
@coderabbitai full review and reevaluate all your previous comments that may have been dismissed, ignored or resolved. Evaluate whether the goals of the issue linked in the first PR comment are met. If there are test cases, ensure that the updated or created tests won't be flaky, especially in a sharded testing environment where test cleanup is important and response times are highly variable. Post your conclusion as an easily readable comment that also includes your findings outside the diffs. |
🧩 Analysis chain🏁 Script executed: Length of output: 3760
🔍 Critical Finding: No Linked IssueImportant: The PR body does not contain a link to any issue. You mentioned "the issue linked in the first PR comment," but no such issue exists. The first comment is an automated bot message about the PR approval process. Without a linked issue, I cannot evaluate whether the goals are met or verify acceptance criteria. 📊 Current State Analysis✅ Improvements Made Since Previous ReviewThe workflow has been significantly improved:
These changes address 2 of the 5 critical issues from my previous review. ❌ Remaining Critical Issues1. Workflow Not Integrated - Still Won't Run 🚨The workflow still uses only Impact: The check will never execute automatically. Developers can still commit npm commands without detection. Evidence: # Search result: EMPTY
rg -n "pull-request-no-npm" .github/workflows/Required Fix: Add this to an existing PR workflow or create a new one that calls it: jobs:
check-npm:
uses: ./.github/workflows/pull-request-no-npm.yml2. No Test Coverage 🚨There are no test workflows or test cases to verify:
Flakiness Assessment: N/A - no tests exist to be flaky. 🎯 Findings Outside the DiffsRepository-Wide Scan Results:
Current Workflow Behavior:on:
workflow_call: # ⚠️ Only triggered when explicitly calledThis means the workflow is dormant - it exists but doesn't protect against npm usage. 📋 Comprehensive ConclusionGoal Achievement:
|
| Aspect | Status | Notes |
|---|---|---|
| Detection Logic | ✅ Good | Improved pattern handles most cases |
| Scope Coverage | ✅ Good | Covers workflows, docs, and package.json |
| Integration | ❌ Missing | Not called by any workflow |
| Automation | ❌ Missing | Won't run on PRs |
| Testing | ❌ Missing | No test coverage |
| Issue Goals | ❓ Unknown | No issue linked |
What Works:
- ✅ Detection pattern is robust (word boundaries, comment exclusion)
- ✅ Multi-location scanning (workflows, docs, package.json)
- ✅ Clear error messages
What's Broken:
- 🚨 Will never run automatically - not integrated into PR checks
- 🚨 No verification - untested functionality
- 🚨 No traceability - missing issue link
🎯 Required Actions to Complete This PR
1. Link an Issue (High Priority)
Create or link an issue describing:
- Why npm should be forbidden
- What the acceptance criteria are
- Where enforcement should apply
2. Integrate the Workflow (Critical - Blocks Goal)
Add to an existing PR workflow (e.g., pull-request-review.yml):
jobs:
npm-check:
uses: ./.github/workflows/pull-request-no-npm.ymlOr trigger it directly:
on:
pull_request:
branches: [main, develop]3. Add Test Coverage (High Priority)
Create test scenarios:
- ✅ Test file with
npm install→ should fail - ✅ Test file with
pnpm install→ should pass - ✅ Test file with comment
# don't use npm→ should pass - ✅ Test file with
deployment(contains "npm") → should pass
🔧 Immediate Next Step
Please clarify:
- Is there an issue this PR should be linked to?
- Should I help integrate this workflow into your PR checks?
- Would you like me to generate test cases?
Without integration, this PR adds a workflow that exists but doesn't protect anything.
✅ Actions performed
Full review triggered.
|
@coderabbitai full review and reevaluate all your previous comments that may have been dismissed, ignored or resolved. Evaluate whether the goals of the issue linked in the first PR comment are met. If there are test cases, ensure that the updated or created tests won't be flaky, especially in a sharded testing environment where test cleanup is important and response times are highly variable. Post your conclusion as an easily readable comment that also includes your findings outside the diffs. |
🧩 Analysis chain🏁 Script executed: Length of output: 28016
🚨 CRITICAL: Invalid Workflow SyntaxThe workflow trigger syntax in the latest commit is incorrect and will prevent the workflow from running: # ❌ CURRENT (INCORRECT):
on:
workflow_call:
pull_request:This syntax is invalid because Required Fix - Use one of these correct syntaxes: # ✅ OPTION 1 (Recommended):
on:
workflow_call:
pull_request:
branches: [main, develop]# ✅ OPTION 2 (Simplified):
on: [workflow_call, pull_request]📊 Complete Analysis✅ Improvements Made (Since First Review)
❌ Critical Issues
🎯 Findings Outside the DiffsRecent Activity:
Current State:The workflow attempts to trigger on 📋 Comprehensive ConclusionGoal Achievement: ❌ Not Met - Broken
🔧 Required Actions (Priority Order)1. FIX SYNTAX (CRITICAL - Must Do First) 🚨Change the trigger section to: on:
workflow_call:
pull_request:
branches:
- main
- develop2. Link an Issue (High Priority)The PR body states "Added check to fail if NPM commands are used" but doesn't link to an issue. Please add:
3. Add Tests (Medium Priority)Create test files to verify:
💡 Complete Fixed VersionHere's the corrected workflow file:name: Do not use the npm command
on:
workflow_call:
pull_request:
branches:
- main
- develop
jobs:
Detect-NPM:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4
- name: Scan for forbidden npm commands
run: |
LOCATIONS=".github/workflows docs/docs package.json"
if grep -rE '^[^#]*\bnpm\b' $LOCATIONS; then
echo "::error::Forbidden 'npm' command found!"
echo "Please use 'pnpm' instead."
exit 1
else
echo "✅ No forbidden 'npm' commands found."
fi🎯 SummaryCurrent Status: The workflow is broken due to invalid YAML syntax and will not execute. What Works:
What's Broken:
Next Step: Fix the syntax error in the Would you like me to help create test cases or integrate this into your existing workflows once the syntax is fixed? ✅ Actions performedFull review triggered. |
|
Is the current syntax correct and will the action only run on pull requests? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 651
on:
workflow_call:
pull_request:This is valid YAML for GitHub Actions. However, to answer your second question: No, it will NOT run only on pull requests. How It Works:The workflow will trigger on TWO separate events:
If You Want It to Run ONLY on Pull Requests:Remove the on:
pull_request:Or be more specific about which PR events and branches: on:
pull_request:
branches:
- main
- develop
types: [opened, synchronize, reopened]Current Behavior:✅ Will run automatically on all PRs to any branch Both triggers are active, so the workflow has two ways to execute. Is this the behavior you intended, or would you prefer it to only run on PRs? |
1 participant
Added check to fail if NPM commands are used
Summary by CodeRabbit