Visitar URL original
chore(deps): approve npm install scripts and fix doctor staleness warns by junkovich · Pull Request #782 · unkos-dev/reverie · GitHub
Skip to content

chore(deps): approve npm install scripts and fix doctor staleness warns - #782

Merged
junkovich merged 4 commits into
mainfrom
chore/approve-install-scripts
Jul 30, 2026
Merged

junkovich merged 4 commits into
mainfrom
chore/approve-install-scripts

Conversation

@junkovich

Copy link
Copy Markdown
Contributor

npm 11.18 gates dependency install scripts on an allowScripts allowlist in the root package.json and skips any unreviewed script with a warning, which the vp install that just doctor advises surfaced for esbuild, lefthook, and puppeteer.

  • Approve the three reviewed scripts as pinned package@version entries (npm's default), so a version bump surfaces the new version's script for review instead of inheriting trust. esbuild and lefthook only validate binaries their platform packages already ship, but puppeteer's script downloads its pinned Chrome build; without approval a fresh checkout fails at the accessibility scan with no signal at install time. The approval flow is documented in the Dependencies section of CONTRIBUTING.
  • Fix the doctor warn labels for the node_modules staleness check. The warn branches reused the pass-state label, so a stale tree printed "node_modules matches package-lock.json" while warning about the opposite. The labels now state the finding (stale tree, or incomplete install for the missing-marker branch), with the selftest assertions updated to match.
  • Correct the frontend-only setup instructions, which predate the workspace hoist: npm install inside frontend/ treats that directory as its own root and creates a stray nested node_modules and lockfile. Install at the repository root and start the dev server with npm run dev --workspace frontend.

Verification: npm install-scripts ls reports nothing pending and vp install runs clean; npm rebuild esbuild lefthook confirms approved scripts execute; scripts/doctor-selftest.sh passes; just preflight is green (13 lanes).

…uppeteer

npm 11.18 gates install scripts on an allowScripts allowlist and skips
unreviewed scripts with a warning. The skipped postinstalls are harmless
on a warmed machine because esbuild and lefthook ship their binaries as
platform packages, but a fresh checkout never downloads puppeteer's
pinned Chrome, so the accessibility scan fails at first use with no
signal at install time. Approve the three reviewed scripts, pinned to
exact versions so a bump surfaces the new version's script for review,
and document the approval flow for contributors.

Signed-off-by: junkovich <junkovich@users.noreply.github.com>
…bels

The warn branches reused the check's pass-state label, so a stale tree
printed 'node_modules matches package-lock.json' as a warning, asserting
the opposite of what was found. Name the finding itself: a stale tree in
the two staleness branches and an incomplete install in the missing
marker branch.

Signed-off-by: junkovich <junkovich@users.noreply.github.com>
…nly path

The frontend-only instructions predate the workspace hoist: running
npm install inside frontend/ treats that directory as its own root and
creates a stray nested node_modules and lockfile the tooling never
reads. Install at the root and run the dev script through the
workspace flag instead.

Signed-off-by: junkovich <junkovich@users.noreply.github.com>
@github-actions github-actions Bot added dependencies Dependency updates (cargo, npm, Docker base images) area/infra GitHub workflows, Docker, repo infrastructure labels Jul 30, 2026
@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

The PR successfully addresses the security requirements for npm script approvals and updates the contributor documentation for the monorepo setup. Codacy analysis indicates the changes are 'up to standards' with no new quality issues detected. However, a significant gap exists in the diagnostic label updates for scripts/doctor.sh. Despite the PR's stated goal to ensure warning messages state the actual finding, several branches (specifically lines 144 and 160) still use success-state labels (like 'present' or 'matches') in failure conditions. Additionally, a command typo was identified in the documentation that could hinder the contributor onboarding process. These items should be addressed to fully meet the acceptance criteria.

About this PR

  • There is a systemic pattern of incomplete diagnostic label updates in scripts/doctor.sh. While some labels were fixed, several warning branches still utilize success-state strings (e.g., reporting a component is 'present' when it was found to be missing). This contradicts the PR's goal to improve diagnostic clarity.

Test suggestions

  • Verify doctor.sh reports 'node_modules stale against package-lock.json' when the lockfile is newer than the install marker
  • Verify doctor.sh reports 'node_modules install incomplete (marker missing)' when the install marker is absent
  • Verify doctor-selftest.sh successfully validates the updated warning labels for staleness and missing markers

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

Comment thread scripts/doctor.sh
# --no-install has nothing to fall back to in that case either.
if [ -x node_modules/.bin/vp ]; then
warn "node_modules matches package-lock.json" "npx --no-install vp install"
warn "node_modules stale against package-lock.json" "npx --no-install vp install"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: This is a good fix for the misleading diagnostic labels, but it has not been applied exhaustively. Lines 144 and 160 still exhibit the 'copy-paste' error of using a success string in a warning branch (e.g., line 160 warns that node_modules is 'present' when it has just been found to be missing). Update these warn messages to accurately describe the failure state and ensure the corresponding expect_contains assertions in scripts/doctor-selftest.sh are updated to match.

Comment thread .github/CONTRIBUTING.md

New Rust dependencies must satisfy the supply-chain policy in [`backend/deny.toml`](../backend/deny.toml): a crate whose license is outside the permissive allowlist (any GPL/LGPL/AGPL or otherwise unlisted license) or that resolves to a git source rather than crates.io will fail the `cargo deny check` run in the `audit` CI job. If you have a legitimate need for such a dependency, raise it in the PR so the policy exception can be reviewed.

npm install scripts are gated by the `allowScripts` field in the root `package.json`: npm skips any install script not approved there and prints a warning naming the package. Approvals are pinned to exact versions, so a version bump of an approved package warns again until the new version is re-approved. If a dependency legitimately needs its install script, review what the script does, then run `npm install-scripts approve <pkg>` and commit the resulting `package.json` change in the same PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: The command npm install-scripts is likely a typo. Use npm run install-scripts if it is a script defined in package.json, or npx allow-scripts if you are referring to the underlying utility directly.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The contribution guide now instructs developers to install dependencies and start the frontend through the root npm workspace command, and documents install-script approval using allowScripts. Doctor diagnostics now identify stale node_modules and incomplete installs with more specific warning messages. The doctor self-test expectations were updated for the revised wording while retaining the existing remediation commands.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7f3ce936-3783-442f-a840-c110a52b6809

📥 Commits

Reviewing files that changed from the base of the PR and between 54b8797 and 238fb20.

⛔ Files ignored due to path filters (1)
  • package.json is excluded by none and included by none
📒 Files selected for processing (3)
  • .github/CONTRIBUTING.md
  • scripts/doctor-selftest.sh
  • scripts/doctor.sh

Comment thread .github/CONTRIBUTING.md Outdated
Comment on lines +89 to +92
Frontend only (Node.js at or above the `engines.node` floor in `package.json`; install at the repository root, where npm workspaces hoist every plane's dependencies):

```bash
cd frontend && npm install && npm run dev
npm install && npm run dev --workspace frontend

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Files:"
git ls-files | rg '(^|/)(package\.json|package-lock\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lockb|CONTRIBUTING\.md)$|\.github/CONTRIBUTING\.md'

echo
echo "Package manager files and relevant package manager config:"
for f in package.json package-lock.json pnpm-lock.yaml yarn.lock bun.lockb .github/CONTRIBUTING.md; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    case "$f" in
      package.json) sed -n '1,220p' "$f" ;;
      CONTRIBUTING.md|.github/CONTRIBUTING.md) sed -n '1,140p' "$f" ;;
      *) sed -n '1,80p' "$f" ;;
    esac
  fi
done

echo
echo "Search for npm/ci/install/workspace docs:"
rg -n "npm (ci|install)|lockfile|workspace|restore|dependencies|pnpm|yarn|bun" . -g '!node_modules' -g '!dist' -g '!build' | head -n 200

Repository: unkos-dev/reverie

Length of output: 43971


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
import json, subprocess
from pathlib import Path

package_path = Path("package.json")
contrib_path = Path(".github/CONTRIBUTING.md")
lock_paths = [Path("package-lock.json"), Path("npm-shrinkwrap.json"), Path("pnpm-lock.yaml"), Path("yarn.lock"), Path("bun.lockb")]

print("package_json_exists:", package_path.exists())
if package_path.exists():
    data = json.loads(package_path.read_text())
    print("package_engines_node:", data.get("engines", {}).get("node"))
    print("package_workspaces:", data.get("workspaces"))
    print("package_scripts_dev_frontend:", data.get("scripts", {}).get("dev"))
    print("package_scripts_dev_workspace:", data.get("scripts", {}).get("dev --workspace frontend"))
    print("frontend_package_exists:", (package_path.parent / "frontend" / "package.json").exists())
    ff = package_path.parent / "frontend" / "package.json"
    if ff.exists():
        fd = json.loads(ff.read_text())
        print("frontend_package_name:", fd.get("name"))
        print("frontend_package_dev_script:", fd.get("scripts", {}).get("dev"))

print("lockfile_exists:", {str(p): p.exists() for p in lock_paths})

contrib_text = contrib_path.read_text() if contrib_path.exists() else ""
for needle in ["npm install", "npm ci", "npm update", "npm run dev --workspace frontend", "lockfile"]:
    print(f"contrib_contains_{needle!r}:", needle in contrib_text)

# npm semantics check: if npm package manager is present and the npm version is semver-compatible enough,
# ask npm to document lockfile-backed install command without mutating repository files.
try:
    res = subprocess.run(["npm", "config", "get", "update-notifier"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True, text=True, timeout=20)
    npm_exists = True
except Exception as e:
    npm_exists = False
print("npm_exists:", npm_exists)
if npm_exists:
    print("npm_config_update-notifier:", res.stdout.strip())
PY

Repository: unkos-dev/reverie

Length of output: 779


Use the lockfile-backed install command

The frontend setup uses npm install, while the repository already documents and relies on npm ci for lockfile-backed dependency restoration. Use npm ci at the repository root so contributors do not risk rewriting the committed npm lockfile.

Suggested change
-npm install && npm run dev --workspace frontend
+npm ci && npm run dev --workspace frontend
📝 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.

Suggested change
Frontend only (Node.js at or above the `engines.node` floor in `package.json`; install at the repository root, where npm workspaces hoist every plane's dependencies):
```bash
cd frontend && npm install && npm run dev
npm install && npm run dev --workspace frontend
Frontend only (Node.js at or above the `engines.node` floor in `package.json`; install at the repository root, where npm workspaces hoist every plane's dependencies):

Source: Coding guidelines

Comment thread .github/CONTRIBUTING.md
Comment on lines +178 to +179

npm install scripts are gated by the `allowScripts` field in the root `package.json`: npm skips any install script not approved there and prints a warning naming the package. Approvals are pinned to exact versions, so a version bump of an approved package warns again until the new version is re-approved. If a dependency legitimately needs its install script, review what the script does, then run `npm install-scripts approve <pkg>` and commit the resulting `package.json` change in the same PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '--- CONTRIBUTING relevant lines ---\n'
sed -n '160,190p' .github/CONTRIBUTING.md 2>/dev/null || true

printf '\n--- package.json files ---\n'
git ls-files '*package.json' | sed -n '1,50p'

printf '\n--- root package.json allowScripts/workspaces ---\n'
python3 - <<'PY'
import json
with open('package.json') as f:
    data=json.load(f)
print('workspaces:', data.get('workspaces'))
print('allowScripts:', data.get('allowScripts'))
PY

printf '\n--- npm install-scripts references ---\n'
rg -n "install-scripts|allowScripts" -S . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**'

Repository: unkos-dev/reverie

Length of output: 246


🌐 Web query:

npm install-scripts command workspaces allowScripts npm install-scripts not workspace aware

💡 Result:

The npm install-scripts command is not workspace-aware, meaning it does not operate across individual workspaces independently; instead, it manages the allowScripts configuration centrally at the root level [1][2]. Key points regarding workspaces and install-script security: 1. Centralized Management: In a monorepo or workspace-based project, the allowScripts field in the root package.json serves as the single source of truth for the entire workspace [1]. Individual workspaces (sub-packages) do not manage their own allowScripts policies; if the field is present in a non-root package.json, npm will ignore it and issue a warning [1][3]. 2. Command Scope: The npm install-scripts command interacts with this root configuration to approve or deny dependencies for the entire project [1][2]. It is specifically documented as being unaware of workspaces in terms of granular, per-workspace configuration [2]. 3. Security Policy: The transition to opt-in install scripts (governed by allowScripts) is designed to be a project-wide security policy [1]. Whether you run npm install from the root or from within a sub-workspace, the policy defined in the root package.json is the one enforced [1]. If you are encountering issues where install scripts are not running as expected within workspaces, note that lifecycle scripts (such as preinstall, install, and postinstall) generally run when packages are installed, but they are subject to the root-level allowScripts policy [1][4]. If scripts fail to execute in a workspace context, it may be due to the project's security configuration or specific CLI version behaviors [5][6]. To manage script permissions for your entire project, always target the root package.json [1].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- CONTRIBUTING relevant lines ---'
sed -n '160,190p' .github/CONTRIBUTING.md 2>/dev/null || true

printf '%s\n' ''
printf '%s\n' '--- package.json files ---'
git ls-files '*package.json' | sed -n '1,50p'

printf '%s\n' ''
printf '%s\n' '--- root package.json allowScripts/workspaces ---'
python3 - <<'PY'
import json
with open('package.json') as f:
    data=json.load(f)
print('workspaces:', data.get('workspaces'))
print('allowScripts:', data.get('allowScripts'))
PY

printf '%s\n' ''
printf '%s\n' '--- npm install-scripts references ---'
rg -n "install-scripts|allowScripts" -S . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**'

Repository: unkos-dev/reverie

Length of output: 3754


Run npm install-scripts approve from the repository root

NPM applies the allowScripts security policy in the root package.json, so the approval command should be documented as a repository-root operation to avoid contributors running it from frontend/ or docs/.

…only path

npm install can rewrite the committed lockfile when a checkout has
drifted; npm ci restores exactly what the lockfile declares, matching
the deterministic-install rule the supply-chain policy already applies
in CI.

Signed-off-by: junkovich <junkovich@users.noreply.github.com>
@sonarqubecloud

Copy link
Copy Markdown

@junkovich
junkovich merged commit 59069af into main Jul 30, 2026
42 checks passed
@junkovich
junkovich deleted the chore/approve-install-scripts branch July 30, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/infra GitHub workflows, Docker, repo infrastructure dependencies Dependency updates (cargo, npm, Docker base images)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant