fix: install nvm and node for -ui cli command #1836
Conversation
Please make sure all the checkboxes are checked:
|
WalkthroughUpdates frontend dependencies (Next.js 16, React 19.2, Auth0), refactors React ref types from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
nit: most of the changes in the frontend seem not related, maybe can be split? |
Frontend had some critical dependency issues, so took a chance to fix that as well. Frontend can be also built now, don't need to be served as dev version. |
![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
♻️ Duplicate comments (1)
cognee/api/v1/ui/node_setup.py (1)
91-91: Update nvm version to latest stable.As noted in a previous review, the nvm version
v0.39.7is outdated.
🧹 Nitpick comments (12)
cognee-frontend/src/app/dashboard/page.tsx (1)
1-11: Client page implementation is correct; consider whether top-leveluse clientis neededThis component is syntactically and semantically fine as a client page and will render
Dashboardas expected. IfPageitself doesn’t need hooks or browser-only APIs andDashboardis already a client component, you could keepPageas a server component (remove"use client"and keep a plain/default export) for slightly better SSR behavior. If the intent is to make the entire dashboard route client-only for consistency with other changes in this PR, the current version is OK as-is.cognee/api/v1/ui/npm_utils.py (2)
37-47: Potential shell injection risk when joining command arguments.Line 40 uses
' '.join(cmd)to construct a shell command string, which could be problematic if any element incmdcontains spaces, special characters, or shell metacharacters. While this is an internal utility, consider usingshlex.join()for safer command construction.+import shlex + from cognee.shared.logging_utils import get_logger from .node_setup import get_nvm_sh_path- nvm_cmd = f"source {nvm_path} && {' '.join(cmd)}" + nvm_cmd = f"source {nvm_path} && {shlex.join(cmd)}"
12-50: Missingsubprocess.TimeoutExpiredexception handling.The function specifies a
timeoutparameter but doesn't handle theTimeoutExpiredexception. The calling code inui.pyhandles it, but this function should either handle it internally for consistency or document that callers must handle it.Consider adding exception handling or documenting the expected behavior:
def run_npm_command(cmd: List[str], cwd: Path, timeout: int = 300) -> subprocess.CompletedProcess: """ Run an npm command, ensuring nvm is sourced if needed (Unix-like systems only). Returns the CompletedProcess result. + + Raises: + subprocess.TimeoutExpired: If the command does not complete within the timeout. """cognee/api/v1/ui/node_setup.py (3)
126-131: Bareexceptclause withpasssilently swallows all errors.Line 130 uses a bare
except Exceptionwithpass, which could hide important cleanup failures. Consider logging at debug level.finally: # Clean up temporary script try: os.unlink(install_script_path) - except Exception: - pass + except Exception as e: + logger.debug(f"Failed to clean up temp script: {e}")
210-360: Significant code duplication incheck_node_npmfunction.This function is ~150 lines with substantial duplicated patterns:
- The "try direct command, then try with nvm sourced" pattern appears 4+ times
- The nvm/node installation fallback logic is duplicated in both the main
tryblock andFileNotFoundErrorhandler- Similar error handling and logging throughout
Consider extracting helper functions to reduce duplication and improve maintainability.
Example refactor approach:
def _run_with_nvm_fallback(cmd: List[str], timeout: int = 10) -> subprocess.CompletedProcess: """Run a command, falling back to nvm-sourced execution if direct fails.""" result = subprocess.run(cmd, capture_output=True, text=True, timeout=timeout) if result.returncode != 0: nvm_path = get_nvm_sh_path() if nvm_path.exists(): result = subprocess.run( ["bash", "-c", f"source {nvm_path} && {' '.join(cmd)}"], capture_output=True, text=True, timeout=timeout, ) return result def _ensure_node_installed() -> tuple[bool, str]: """Ensure Node.js is installed, installing via nvm if needed.""" # Consolidated installation logic here ...
312-358: Near-duplicate logic inFileNotFoundErrorhandler.The
FileNotFoundErrorexception handler (lines 314-358) largely duplicates the logic in the main try block (lines 231-274) for installing nvm and Node.js. This violates DRY principles.Consider consolidating into a single code path by catching
FileNotFoundErroralongside the returncode check, or extracting the installation logic into a helper function.cognee/api/v1/ui/ui.py (2)
588-601: Duplicated version-finding logic withnode_setup.py.This PATH augmentation logic (lines 594-601) duplicates the version-finding code from
install_node_with_nvm()innode_setup.py(lines 186-195). Both have the same lexicographic sorting issue that may not correctly identify the "latest" version.Consider extracting this into a shared helper function in
node_setup.py.Add a helper function to
node_setup.py:def get_nvm_node_bin_path() -> Optional[Path]: """Get the bin path for the latest nvm-installed Node.js version.""" nvm_versions = get_nvm_dir() / "versions" / "node" if nvm_versions.exists(): versions = sorted(nvm_versions.iterdir(), reverse=True) if versions: bin_path = versions[0] / "bin" if bin_path.exists(): return bin_path return NoneThen use it in both
ui.pyandnode_setup.py.
610-639: Consider consolidating npm execution logic.Both branches (Windows at lines 610-618 and Unix with nvm at lines 620-630) could potentially be simplified by leveraging
run_npm_commandfromnpm_utils.pywith aPopen-compatible variant, though this may require additional abstraction.cognee-frontend/src/app/(graph)/GraphView.tsx (1)
3-3: Ref setup looks correct; consider dropping casts and making handlers null‑safeThe
useRef<...>(null)migration forgraphRef,graphControls, andactivityLogis consistent with React 19 ref typing, and the newRefObjectimport is appropriate. The repeatedas RefObject<...>casts when passing refs toGraphVisualization,ActivityLog, andGraphControlsare likely unnecessary; TypeScript should accept the refs directly. Also, usinggraphRef.current!in the handlers can be made a bit safer with optional chaining so calls are simply no‑ops if the visualization isn’t ready yet.Example refactor (if TS types allow):
- const graphRef = useRef<GraphVisualizationAPI>(null); - const graphControls = useRef<GraphControlsAPI>(null); - const activityLog = useRef<ActivityLogAPI>(null); + const graphRef = useRef<GraphVisualizationAPI>(null); + const graphControls = useRef<GraphControlsAPI>(null); + const activityLog = useRef<ActivityLogAPI>(null); - <GraphVisualization + <GraphVisualization key={data?.nodes.length} - ref={graphRef as RefObject<GraphVisualizationAPI>} + ref={graphRef} data={data} - graphControls={graphControls as RefObject<GraphControlsAPI>} + graphControls={graphControls} /> ... - <ActivityLog ref={activityLog as RefObject<ActivityLogAPI>} /> + <ActivityLog ref={activityLog} /> ... - <GraphControls + <GraphControls data={data} - ref={graphControls as RefObject<GraphControlsAPI>} + ref={graphControls} isAddNodeFormOpen={isAddNodeFormOpen} - onFitIntoView={() => graphRef.current!.zoomToFit(1000, 50)} - onGraphShapeChange={(shape) => graphRef.current!.setGraphShape(shape)} + onFitIntoView={() => graphRef.current?.zoomToFit(1000, 50)} + onGraphShapeChange={(shape) => graphRef.current?.setGraphShape(shape)} />If TypeScript still complains without the casts, keeping only the
?.change would already improve robustness.Also applies to: 50-55, 75-100
cognee-frontend/src/app/(graph)/GraphVisualization.tsx (1)
4-5: RefObject‑based API for GraphVisualization is consistent with callers; cast could be simplifiedUsing
RefObject<GraphVisualizationAPI>/RefObject<GraphControlsAPI>on the props and initializinggraphRefwithuseRef<ForceGraphMethods>(null)lines up cleanly with howGraphViewandNotebookare now consuming this component. ThezoomToFitwrapper and its null guard are appropriate given the async nature of the ForceGraph ref.The cast on the ForceGraph ref is probably not needed:
- const graphRef = useRef<ForceGraphMethods>(null); ... - <ForceGraph - ref={graphRef as RefObject<ForceGraphMethods>} + const graphRef = useRef<ForceGraphMethods>(null); + ... + <ForceGraph + ref={graphRef}If the library’s
reftyping is narrower, keeping the cast is fine; functionally the current code is sound.Also applies to: 18-22, 30-37, 208-209, 243-244
cognee-frontend/src/ui/elements/Notebook/NotebookCellHeader.tsx (1)
36-36: runInstance doesn’t need state; compute it directly from the environmentSince
runInstanceis never updated and the UI doesn’t currently allow switching between local/cloud (the Select is commented out), you can simplify by removinguseStateentirely and deriving it once per render:- const [runInstance] = useState<string>(isCloudEnvironment() ? "cloud" : "local"); + const runInstance: "cloud" | "local" = isCloudEnvironment() ? "cloud" : "local";This keeps behavior the same while avoiding an unnecessary piece of state.
Also applies to: 59-69
cognee-frontend/src/ui/elements/Notebook/Notebook.tsx (1)
5-5: GraphVisualization ref wiring in CellResult is correct but currently unusedThe new
useRef<GraphVisualizationAPI>(null)plusRefObjectimport are consistent with the updatedGraphVisualizationAPI, and the stubbedgraphControlsimplementation prevents undefined‑access issues in this notebook context.However,
CellResultnever readsgraphRef.current, so the ref is effectively unused and only serves to satisfy therefprop type. If you don’t plan to control these embedded graphs imperatively (e.g., callingzoomToFit), you could either:
- Keep the current pattern as is (harmless but a bit noisy), or
- Replace
graphRefwith a simple dummy ref (e.g.,useRef<GraphVisualizationAPI | null>(null)without exporting it from the component) and clearly comment that it’s intentionally unused, to avoid confusion for future readers.Functionally, no changes are required; this is just a clarity/cleanup consideration.
Also applies to: 282-305, 343-353, 377-383
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cognee-frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
cognee-frontend/package.json(2 hunks)cognee-frontend/src/app/(graph)/CrewAITrigger.tsx(0 hunks)cognee-frontend/src/app/(graph)/GraphControls.tsx(1 hunks)cognee-frontend/src/app/(graph)/GraphView.tsx(3 hunks)cognee-frontend/src/app/(graph)/GraphVisualization.tsx(5 hunks)cognee-frontend/src/app/dashboard/page.tsx(1 hunks)cognee-frontend/src/app/page.tsx(1 hunks)cognee-frontend/src/modules/ingestion/useDatasets.ts(0 hunks)cognee-frontend/src/proxy.ts(1 hunks)cognee-frontend/src/ui/Partials/FeedbackForm.tsx(0 hunks)cognee-frontend/src/ui/Partials/index.ts(0 hunks)cognee-frontend/src/ui/elements/Notebook/Notebook.tsx(5 hunks)cognee-frontend/src/ui/elements/Notebook/NotebookCellHeader.tsx(1 hunks)cognee-frontend/tsconfig.json(2 hunks)cognee/api/v1/ui/node_setup.py(1 hunks)cognee/api/v1/ui/npm_utils.py(1 hunks)cognee/api/v1/ui/ui.py(4 hunks)
💤 Files with no reviewable changes (4)
- cognee-frontend/src/modules/ingestion/useDatasets.ts
- cognee-frontend/src/app/(graph)/CrewAITrigger.tsx
- cognee-frontend/src/ui/Partials/index.ts
- cognee-frontend/src/ui/Partials/FeedbackForm.tsx
🧰 Additional context used
📓 Path-based instructions (4)
{cognee-mcp,cognee-frontend}/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow local README.md and ESLint/TypeScript configuration in cognee-frontend/ for MCP server and Frontend code
Files:
cognee-frontend/src/proxy.tscognee-frontend/src/app/dashboard/page.tsxcognee-frontend/src/ui/elements/Notebook/NotebookCellHeader.tsxcognee-frontend/src/app/page.tsxcognee-frontend/src/ui/elements/Notebook/Notebook.tsxcognee-frontend/src/app/(graph)/GraphVisualization.tsxcognee-frontend/src/app/(graph)/GraphControls.tsxcognee-frontend/src/app/(graph)/GraphView.tsx
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indentation in Python code
Use snake_case for Python module and function names
Use PascalCase for Python class names
Use ruff format before committing Python code
Use ruff check for import hygiene and style enforcement with line-length 100 configured in pyproject.toml
Prefer explicit, structured error handling in Python code
Files:
cognee/api/v1/ui/npm_utils.pycognee/api/v1/ui/node_setup.pycognee/api/v1/ui/ui.py
⚙️ CodeRabbit configuration file
**/*.py: When reviewing Python code for this project:
- Prioritize portability over clarity, especially when dealing with cross-Python compatibility. However, with the priority in mind, do still consider improvements to clarity when relevant.
- As a general guideline, consider the code style advocated in the PEP 8 standard (excluding the use of spaces for indentation) and evaluate suggested changes for code style compliance.
- As a style convention, consider the code style advocated in CEP-8 and evaluate suggested changes for code style compliance.
- As a general guideline, try to provide any relevant, official, and supporting documentation links to any tool's suggestions in review comments. This guideline is important for posterity.
- As a general rule, undocumented function definitions and class definitions in the project's Python code are assumed incomplete. Please consider suggesting a short summary of the code for any of these incomplete definitions as docstrings when reviewing.
Files:
cognee/api/v1/ui/npm_utils.pycognee/api/v1/ui/node_setup.pycognee/api/v1/ui/ui.py
cognee/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Public APIs should be type-annotated in Python where practical
Files:
cognee/api/v1/ui/npm_utils.pycognee/api/v1/ui/node_setup.pycognee/api/v1/ui/ui.py
cognee/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use shared logging utilities from cognee.shared.logging_utils in Python code
Files:
cognee/api/v1/ui/npm_utils.pycognee/api/v1/ui/node_setup.pycognee/api/v1/ui/ui.py
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:45:09.996Z
Learnt from: CR
Repo: topoteretes/cognee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:45:09.996Z
Learning: Applies to {cognee-mcp,cognee-frontend}/**/*.{js,ts,tsx} : Follow local README.md and ESLint/TypeScript configuration in cognee-frontend/ for MCP server and Frontend code
Applied to files:
cognee-frontend/tsconfig.jsoncognee-frontend/package.json
📚 Learning: 2025-11-24T16:45:09.996Z
Learnt from: CR
Repo: topoteretes/cognee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:45:09.996Z
Learning: Applies to cognee/**/*.py : Use shared logging utilities from cognee.shared.logging_utils in Python code
Applied to files:
cognee/api/v1/ui/ui.py
🧬 Code graph analysis (6)
cognee/api/v1/ui/npm_utils.py (2)
cognee/shared/logging_utils.py (1)
get_logger(212-224)cognee/api/v1/ui/node_setup.py (1)
get_nvm_sh_path(25-29)
cognee-frontend/src/ui/elements/Notebook/NotebookCellHeader.tsx (1)
cognee-frontend/src/utils/isCloudEnvironment.ts (1)
isCloudEnvironment(2-4)
cognee-frontend/src/ui/elements/Notebook/Notebook.tsx (1)
cognee-frontend/src/app/(graph)/GraphVisualization.tsx (1)
GraphVisualizationAPI(25-28)
cognee-frontend/src/app/(graph)/GraphVisualization.tsx (1)
cognee-frontend/src/app/(graph)/GraphControls.tsx (1)
GraphControlsAPI(22-25)
cognee-frontend/src/app/(graph)/GraphView.tsx (3)
cognee-frontend/src/app/(graph)/GraphVisualization.tsx (1)
GraphVisualizationAPI(25-28)cognee-frontend/src/app/(graph)/GraphControls.tsx (2)
GraphControlsAPI(22-25)GraphControls(66-235)cognee-frontend/src/app/(graph)/ActivityLog.tsx (2)
ActivityLogAPI(11-13)ActivityLog(21-54)
cognee/api/v1/ui/ui.py (2)
cognee/api/v1/ui/node_setup.py (3)
check_node_npm(210-360)get_nvm_dir(14-22)get_nvm_sh_path(25-29)cognee/api/v1/ui/npm_utils.py (1)
run_npm_command(12-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: CLI Tests / CLI Unit Tests
- GitHub Check: CLI Tests / CLI Functionality Tests
- GitHub Check: CLI Tests / CLI Integration Tests
- GitHub Check: Basic Tests / Run Integration Tests
- GitHub Check: Basic Tests / Run Unit Tests
- GitHub Check: End-to-End Tests / Test Entity Extraction
- GitHub Check: End-to-End Tests / Test using different async databases in parallel in Cognee
- GitHub Check: End-to-End Tests / Conversation sessions test (Redis)
- GitHub Check: End-to-End Tests / Concurrent Subprocess access test
- GitHub Check: End-to-End Tests / Test permissions with different situations in Cognee
- GitHub Check: End-to-End Tests / Test multi tenancy with different situations in Cognee
- GitHub Check: End-to-End Tests / S3 Bucket Test
- GitHub Check: End-to-End Tests / Conversation sessions test (FS)
- GitHub Check: End-to-End Tests / Deduplication Test
- GitHub Check: End-to-End Tests / Test Feedback Enrichment
- GitHub Check: End-to-End Tests / Run Telemetry Pipeline Test
- GitHub Check: End-to-End Tests / Server Start Test
🔇 Additional comments (11)
cognee-frontend/package.json (1)
11-31: Framework and type version bumps look coherent; please confirm compatibility and scopeBumping Next to
16.0.4, React/ReactDOM to^19.2.0, Auth0 SDK, React types, andeslint-config-nextkeeps the stack internally consistent, but this is a substantial upgrade surface. Please double‑check against the official Next 16 / React 19 upgrade notes and your local ESLint/TS setup, and confirm this broader frontend upgrade is intentional for a PR aimed at the CLI UI fix. As per coding guidelines, ensure this remains compatible with the existing frontend tooling configuration.cognee-frontend/tsconfig.json (1)
17-37: Verifyjsx: "react-jsx"aligns with Next 16 TS guidanceSwitching
jsxfrom the typical"preserve"to"react-jsx"whilenoEmitis true will mostly influence type‑checking, but Next’s docs have historically recommended"preserve"so that the framework’s own compiler controls the JSX transform. Please confirm this new setting is explicitly recommended for Next 16, and that linting/build still pass cleanly with your local config. The added.next/.../typesincludes look fine and consistent with using Next’s generated types.cognee-frontend/src/app/page.tsx (1)
1-3: Forcingdynamic = "force-dynamic"changes / to fully dynamic; confirm this is requiredMaking the root page fully dynamic will prevent static/ISR caching for
/, which may be appropriate for a live CLI‑driven dashboard but can have performance implications. Please confirm this behavior change is intentional for the UI flow you’re fixing and not broader than necessary.cognee-frontend/src/proxy.ts (1)
5-17: Theproxyfunction export is not currently wired as a middleware entrypointThis code appears to be unused. The
proxyfunction is marked with@typescript-eslint/no-unused-varsand is never imported anywhere in the codebase. There is nomiddleware.tsentry file, no Next.js middleware configuration innext.config.mjs, and theconfig.matcherexport is not referenced.If this is preparatory code for future middleware implementation, consider either:
- Adding a
middleware.tsatcognee-frontend/src/middleware.tsthat properly imports and re-exportsproxyandconfig, or- Removing this dead code if it's no longer needed.
The commented Auth0 authorization logic suggests this may be legacy code from a previous authentication implementation that was disabled.
Likely an incorrect or invalid review comment.
cognee/api/v1/ui/npm_utils.py (1)
1-10: LGTM: Proper module setup with shared logging utilities.The imports are clean and the logger is correctly initialized using the shared logging utilities. Based on learnings, using
get_loggerfromcognee.shared.logging_utilsis the project convention.cognee/api/v1/ui/node_setup.py (2)
1-11: LGTM: Clean imports and logger setup.The module correctly uses shared logging utilities from
cognee.shared.logging_utilsas per project conventions.
14-22: Good implementation of XDG_CONFIG_HOME handling.The
get_nvm_dir()function correctly follows the XDG Base Directory specification by checkingXDG_CONFIG_HOMEbefore falling back to~/.nvm. This aligns with standard nvm installation logic.cognee/api/v1/ui/ui.py (3)
18-19: LGTM: Clean integration of new utility modules.The imports correctly reference the new
node_setupandnpm_utilsmodules, consolidating nvm/node functionality.
304-304: Good use of the newrun_npm_commandabstraction.The
install_frontend_dependenciesfunction now correctly uses the centralized npm command runner, which handles nvm sourcing automatically.
620-639: Good nvm integration for frontend startup.The conditional logic correctly sources nvm when available on Unix-like systems while preserving the direct npm execution fallback. The
preexec_fn=os.setsidensures clean process group handling for termination.cognee-frontend/src/app/(graph)/GraphControls.tsx (1)
112-151: Explicit null initialization for shapeChangeTimeout matches the cleanup logicInitializing
shapeChangeTimeoutwithuseRef<number | null>(null)aligns with theif (shapeChangeTimeout.current) { clearTimeout(...) }checks in the effect and teardown, avoiding any undefined/null ambiguity. The auto‑cycling and cleanup behavior remain correct with this change.
| try: | ||
| # Download and install nvm | ||
| nvm_install_script = "https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.7/install.sh" | ||
| logger.info(f"Downloading nvm installer from {nvm_install_script}...") | ||
|
|
||
| response = requests.get(nvm_install_script, timeout=60) | ||
| response.raise_for_status() | ||
|
|
||
| # Create a temporary script file | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False) as f: | ||
| f.write(response.text) | ||
| install_script_path = f.name | ||
|
|
||
| try: | ||
| # Make the script executable and run it | ||
| os.chmod(install_script_path, 0o755) | ||
| result = subprocess.run( | ||
| ["bash", install_script_path], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
nvm install script verification checksum integrity best practices
💡 Result:
Short checklist — best practices for verifying nvm (or any) install scripts and checksums:
-
Don’t pipe to shell. Download first, review, then run. Example: curl -fsSLo install.sh URL; less install.sh; bash install.sh. This avoids executing unreviewed/partial downloads. [3][4]
-
Verify integrity with a strong checksum (SHA-256) published by a trusted channel before running: compute shasum -a 256 install.sh and compare to the publisher’s value. If the publisher offers a checksum file, fetch it over HTTPS. [1][2]
-
Prefer cryptographic signatures over plain checksums when available (GPG/PKI). Verify the signature and the signer’s key trust chain. If using git tags, prefer signed commits/tags and verify them. [4]
-
Prefer safer install methods: official packages via your distro’s package manager, Homebrew, or cloning the repo and checking out a specific commit/tag you can verify, rather than an ad-hoc curl|bash. nvm’s README shows the curl installer but also documents installing from the repo. [1]
-
Validate transport security and source authenticity: use HTTPS, check the host (github.com), and prefer pinned URLs that include a release tag or commit hash (and verify the tag/commit didn’t change). Beware tag re‑assignment risks. [1][4]
-
Use small blast-radius execution: run installers as an unprivileged user, in a container, or VM when you must run potentially risky scripts. Inspect the script for network/privilege changes and unexpected behavior before granting sudo. [3][5]
-
Automate verification in CI: store and compare expected checksums/signatures in your repo or artifact store; fail the build if verification fails. Log the exact installer version/commit used for audits. [4]
-
Tools and helpers: consider utilities like checksum.sh (or simple shell functions) to fetch+verify before execution, or maintain your own signed release metadata. [2]
References:
- nvm installer and recommendations (official README). [1]
- checksum.sh — example workflow for downloading+verifying install scripts. [2]
- Guidance on risks of curl|bash and safer alternatives. [3][5]
- Notes on git tag reassignments and supply-chain risks / verification best practices. [4]
Security concern: Downloading and executing remote scripts without verification.
This code downloads a shell script from the internet and executes it directly without integrity verification, which follows the risky "curl|bash" pattern. Per official nvm documentation and security best practices, add:
- Checksum verification: Fetch the published SHA-256 checksum for the installer version and verify it before execution (see checksum.sh for implementation patterns).
- User logging/warning: Log explicitly what script version is being downloaded and executed so users can audit the action.
- Consider safer alternatives: nvm's README documents installation directly from the repository (cloning and checking out a specific commit/tag you can verify) as a more secure alternative to the installer script.
🤖 Prompt for AI Agents
In cognee/api/v1/ui/node_setup.py around lines 89 to 110, the code downloads and
executes the nvm install script directly which is unsafe; update it to verify
integrity and improve logging: fetch the expected SHA-256 checksum for the
specific nvm version you plan to install (or include a pinned commit/tag),
compute the checksum of the downloaded script and compare before executing, and
abort with a clear error and logged details if the checksums do not match; also
log the exact nvm version/URL being downloaded and that a checksum check will be
performed; optionally replace the installer execution with a safer alternative
by cloning the nvm repo and checking out a specific tag/commit and verifying
that commit before running any install steps.
| # Update PATH for current process | ||
| nvm_bin = nvm_dir / "versions" / "node" | ||
| # Find the latest installed version | ||
| if nvm_bin.exists(): | ||
| versions = sorted(nvm_bin.iterdir(), reverse=True) | ||
| if versions: | ||
| latest_node_bin = versions[0] / "bin" | ||
| if latest_node_bin.exists(): | ||
| current_path = os.environ.get("PATH", "") | ||
| os.environ["PATH"] = f"{latest_node_bin}:{current_path}" |
There was a problem hiding this comment.
Version sorting may not produce semantic version order.
Using sorted(nvm_bin.iterdir(), reverse=True) sorts directory names lexicographically, not by semantic version. For example, v9.0.0 would sort after v18.0.0. Consider using a semver-aware sort or relying on nvm's current symlink.
- if nvm_bin.exists():
- versions = sorted(nvm_bin.iterdir(), reverse=True)
- if versions:
- latest_node_bin = versions[0] / "bin"
+ # Try to use nvm's 'default' alias symlink first
+ default_bin = nvm_dir / "alias" / "default"
+ if default_bin.exists():
+ # Follow the symlink to get the actual version
+ pass # Implementation depends on nvm's structure
+ elif nvm_bin.exists():
+ # Fallback to sorting (note: lexicographic, not semver)
+ versions = sorted(nvm_bin.iterdir(), reverse=True)
+ if versions:
+ latest_node_bin = versions[0] / "bin"Alternatively, use nvm current to get the active version.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cognee/api/v1/ui/node_setup.py around lines 186 to 195, the code sorts nvm
version directories lexicographically which can mis-order semantic versions;
update the logic to pick the latest version semantically (for example, list the
subdirs, extract/normalize their version strings (strip leading "v"), and select
the max using a semver-aware comparator such as packaging.version.parse or a
semver library), then use that directory's "bin" path; alternatively, prefer
reading nvm's active version via the "current" symlink or by invoking "nvm
current" and use that result as the bin path, and ensure you keep a safe
fallback if parsing or lookup fails.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cognee/api/v1/ui/node_setup.py (2)
141-207: Avoid lexicographic version sorting when choosing the Node version directory.When updating
PATH, you sortnvm_dir / "versions" / "node"lexicographically and pick the first entry:versions = sorted(nvm_bin.iterdir(), reverse=True) if versions: latest_node_bin = versions[0] / "bin"Lexicographic order does not match semantic version order (e.g.,
v9.0.0sorts afterv18.0.0), so you can end up choosing an older Node version if both<10and>=10majors are installed. Prefer either:
- Asking nvm for the active/default version and deriving the directory (e.g. run
nvm currentor read thealias/defaultfile), or- Parsing the directory names semantically, e.g.:
from packaging.version import parse as parse_version versions = [d for d in nvm_bin.iterdir() if d.is_dir()] if versions: latest_dir = max(versions, key=lambda d: parse_version(d.name.lstrip("v"))) latest_node_bin = latest_dir / "bin"This will consistently pick the newest installed Node.
76-139: Security risk: downloading and executing the nvm installer script without verification.
install_nvm()downloads a remote shell script and executes it directly, with no checksum/signature verification or explicit user confirmation. Even with a pinned tag, this is effectively acurl | bashpattern and is a non-trivial supply‑chain risk (previously raised in this file and still present).At minimum, consider:
- Logging a clear warning that a remote script from a specific URL/version is about to be executed.
- Verifying a published checksum or signature for the installer before execution and aborting on mismatch.
- Providing a non-automated fallback (e.g., logging instructions for manual nvm installation) instead of silently running unverified shell code.
🧹 Nitpick comments (1)
cognee/api/v1/ui/node_setup.py (1)
14-29: Consider honoring$NVM_DIRwhen resolving the nvm path.
get_nvm_dir()currently only derives the directory fromXDG_CONFIG_HOMEorHOME, so a user with a customNVM_DIRexport will be treated as if nvm is not installed. Consider preferring the environment variable when present:nvm_dir_env = os.environ.get("NVM_DIR") if nvm_dir_env: return Path(nvm_dir_env) xdg_config_home = os.environ.get("XDG_CONFIG_HOME") if xdg_config_home: return Path(xdg_config_home) / "nvm" return Path.home() / ".nvm"This aligns better with the standard nvm initialization snippet.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cognee/api/v1/ui/node_setup.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indentation in Python code
Use snake_case for Python module and function names
Use PascalCase for Python class names
Use ruff format before committing Python code
Use ruff check for import hygiene and style enforcement with line-length 100 configured in pyproject.toml
Prefer explicit, structured error handling in Python code
Files:
cognee/api/v1/ui/node_setup.py
⚙️ CodeRabbit configuration file
**/*.py: When reviewing Python code for this project:
- Prioritize portability over clarity, especially when dealing with cross-Python compatibility. However, with the priority in mind, do still consider improvements to clarity when relevant.
- As a general guideline, consider the code style advocated in the PEP 8 standard (excluding the use of spaces for indentation) and evaluate suggested changes for code style compliance.
- As a style convention, consider the code style advocated in CEP-8 and evaluate suggested changes for code style compliance.
- As a general guideline, try to provide any relevant, official, and supporting documentation links to any tool's suggestions in review comments. This guideline is important for posterity.
- As a general rule, undocumented function definitions and class definitions in the project's Python code are assumed incomplete. Please consider suggesting a short summary of the code for any of these incomplete definitions as docstrings when reviewing.
Files:
cognee/api/v1/ui/node_setup.py
cognee/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Public APIs should be type-annotated in Python where practical
Files:
cognee/api/v1/ui/node_setup.py
cognee/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use shared logging utilities from cognee.shared.logging_utils in Python code
Files:
cognee/api/v1/ui/node_setup.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: topoteretes/cognee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:45:09.996Z
Learning: Applies to {cognee-mcp,cognee-frontend}/**/*.{js,ts,tsx} : Follow local README.md and ESLint/TypeScript configuration in cognee-frontend/ for MCP server and Frontend code
🧬 Code graph analysis (1)
cognee/api/v1/ui/node_setup.py (1)
cognee/shared/logging_utils.py (3)
get_logger(212-224)debug(209-209)info(205-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Basic Tests / Run Unit Tests
- GitHub Check: Basic Tests / Run Integration Tests
- GitHub Check: Basic Tests / Run Simple Examples
- GitHub Check: CLI Tests / CLI Functionality Tests
- GitHub Check: Basic Tests / Run Basic Graph Tests
- GitHub Check: CLI Tests / CLI Unit Tests
- GitHub Check: CLI Tests / CLI Integration Tests
- GitHub Check: End-to-End Tests / Test Feedback Enrichment
- GitHub Check: End-to-End Tests / Concurrent Subprocess access test
- GitHub Check: End-to-End Tests / Test Entity Extraction
- GitHub Check: End-to-End Tests / Server Start Test
- GitHub Check: End-to-End Tests / Test permissions with different situations in Cognee
- GitHub Check: End-to-End Tests / Run Telemetry Pipeline Test
- GitHub Check: End-to-End Tests / Test multi tenancy with different situations in Cognee
- GitHub Check: End-to-End Tests / S3 Bucket Test
- GitHub Check: End-to-End Tests / Deduplication Test
- GitHub Check: End-to-End Tests / Conversation sessions test (Redis)
- GitHub Check: End-to-End Tests / Conversation sessions test (FS)
🔇 Additional comments (1)
cognee/api/v1/ui/node_setup.py (1)
9-11: Consistent use of shared logger.Using
get_logger()fromcognee.shared.logging_utilskeeps logging consistent with the rest of the project; no issues here. As per coding guidelines, this matches the expected logging conventions.
| def check_nvm_installed() -> bool: | ||
| """ | ||
| Check if nvm (Node Version Manager) is installed. | ||
| """ | ||
| try: | ||
| # Check if nvm is available in the shell | ||
| # nvm is typically sourced in shell config files, so we need to check via shell | ||
| if platform.system() == "Windows": | ||
| # On Windows, nvm-windows uses a different approach | ||
| result = subprocess.run( | ||
| ["nvm", "version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| shell=True, | ||
| ) | ||
| else: | ||
| # On Unix-like systems, nvm is a shell function, so we need to source it | ||
| # First check if nvm.sh exists | ||
| nvm_path = get_nvm_sh_path() | ||
| if not nvm_path.exists(): | ||
| logger.debug(f"nvm.sh not found at {nvm_path}") | ||
| return False | ||
|
|
||
| # Try to source nvm and check version, capturing errors | ||
| result = subprocess.run( | ||
| ["bash", "-c", f"source {nvm_path} && nvm --version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
|
|
||
| if result.returncode != 0: | ||
| # Log the error to help diagnose configuration issues | ||
| if result.stderr: | ||
| logger.debug(f"nvm check failed: {result.stderr.strip()}") | ||
| return False | ||
|
|
||
| return result.returncode == 0 | ||
| except Exception as e: | ||
| logger.debug(f"Exception checking nvm: {str(e)}") | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cognee/api/v1/ui/node_setup.py | head -80Repository: topoteretes/cognee
Length of output: 3000
🏁 Script executed:
python3 << 'EOF'
import subprocess
import sys
# Test 1: Verify shell=True with sequence args behavior
print("=== Test 1: shell=True with sequence args ===")
print("Python version:", sys.version)
print()
# On most systems, let's check what happens with shell=True and a list
print("Subprocess.run documentation notes:")
print("- When shell=True and args is a sequence, Python passes args[0] to shell")
print("- This means with ['nvm', 'version'] + shell=True, only 'nvm' is executed")
print()
# Test with a simple command we can verify
print("Test with 'echo hello world':")
try:
result = subprocess.run(
["echo", "hello", "world"],
capture_output=True,
text=True,
shell=True
)
print(f"Command: ['echo', 'hello', 'world'] with shell=True")
print(f"Output: {result.stdout}")
print(f"Return code: {result.returncode}")
except Exception as e:
print(f"Error: {e}")
print()
print("Test with string instead:")
try:
result = subprocess.run(
"echo hello world",
capture_output=True,
text=True,
shell=True
)
print(f"Command: 'echo hello world' with shell=True")
print(f"Output: {result.stdout}")
print(f"Return code: {result.returncode}")
except Exception as e:
print(f"Error: {e}")
print()
print("=== Test 2: Path with spaces in bash -c ===")
test_path_no_space = "/home/user/.nvm/nvm.sh"
test_path_with_space = "/home/user with space/.nvm/nvm.sh"
print(f"Unquoted path without spaces: source {test_path_no_space}")
print(f"Unquoted path with spaces: source {test_path_with_space}")
print("^ With spaces, bash would interpret it as multiple arguments")
EOFRepository: topoteretes/cognee
Length of output: 148
🌐 Web query:
subprocess.run shell=True sequence args Python documentation
💡 Result:
Short answer: With shell=True you should pass args as a single string. If you pass a sequence, the first item is treated as the command string and any remaining items become arguments to the shell itself (not separate commands). On POSIX the effect is like Popen(['/bin/sh', '-c', args[0], args[1], ...]). Use a loop or join commands into one shell string if you want to run multiple shell commands. [1][2]
Examples:
- Recommended: subprocess.run("ls -l /tmp; echo done", shell=True). [1]
- If you pass a sequence: subprocess.run(["ls -l /tmp; echo done"], shell=True) — the sequence’s first element is given to the shell; additional list items become argv[1:], not additional shell commands. [1][2]
Sources: Python subprocess docs. [1][2]
🌐 Web query:
subprocess.run shell=True Windows behavior nvm command
💡 Result:
Short answer
- On Windows, subprocess.run(..., shell=True) runs your command via the system shell (COMSPEC, normally cmd.exe). That means cmd.exe does the resolution/dispatch, not a POSIX shell. [1]
- That causes two common nvm problems:
- nvm for Unix (the nvm.sh shell function) won’t work from cmd.exe — it’s a shell function that must be sourced in a compatible shell (bash). Use bash -c '. ~/.nvm/nvm.sh; nvm ...' instead. [3]
- nvm-windows may not be on the child process PATH (installer sets NVM_HOME/NVM_SYMLINK and requires a restart or correct PATH), or the Python CreateProcess path lookup behaves differently than cmd.exe. Call nvm by its installed executable path (or ensure NVM_HOME/NVM_SYMLINK are in os.environ['PATH'] for the Python process) or restart so PATH changes take effect. [2][1]
Recommendations
- Prefer calling the exact executable (shutil.which('nvm') or full path) instead of shell=True. This avoids shell semantics and security issues. [4][1]
- If you must use a shell:
- For nvm (POSIX): subprocess.run(['bash','-c', ". ~/.nvm/nvm.sh; nvm use 14; node -v"]) . [3]
- For nvm-windows: ensure PATH contains NVM_HOME and NVM_SYMLINK for the Python process, or run cmd /c "nvm list" (but you may need to use full path or restart after install). [2][1]
Sources
[1] Python subprocess docs (shell=True on Windows / COMSPEC).
[2] nvm-windows installation/FAQ and issues (NVM_HOME/NVM_SYMLINK / cmd vs PowerShell).
[3] Stack Overflow: nvm works in bash but not when executed from Python (nvm is a shell function; source nvm.sh).
[4] Blog/notes on subprocess on Windows: prefer resolving executable (shutil.which) vs using shell=True.
Fix Windows nvm check and quote sourced path for Unix shells.
Two issues here:
-
On Windows,
subprocess.run(["nvm", "version"], shell=True)is incorrect. Withshell=Trueand a sequence, Python passesargs[0]to the shell and remaining items become shell arguments (not command arguments). This means"version"is passed as an argument to the shell itself, not tonvm, potentially mis-detecting installation status.Use either:
result = subprocess.run( ["nvm", "version"], capture_output=True, text=True, timeout=10, )
or:
result = subprocess.run( "nvm version", capture_output=True, text=True, timeout=10, shell=True, )
-
On Unix-like systems,
source {nvm_path}fails ifnvm_pathcontains spaces. Quote the path:cmd = f'source "{nvm_path}" && nvm --version' result = subprocess.run( ["bash", "-c", cmd], capture_output=True, text=True, timeout=10, )
🤖 Prompt for AI Agents
cognee/api/v1/ui/node_setup.py around lines 32-73: the Windows nvm check is
using subprocess.run with shell=True and a sequence which mispasses arguments,
and the Unix `source {nvm_path}` invocation is unquoted so it will break on
paths with spaces. Fix by calling subprocess.run on Windows without shell=True
when passing a list (or alternatively pass a single string and keep shell=True),
e.g. use subprocess.run([...], capture_output=True, text=True, timeout=10)
instead of the current call; and on Unix build the bash -c command with the
nvm_path quoted (wrap the path in double quotes) before passing it to
subprocess.run(["bash","-c", cmd], capture_output=True, text=True, timeout=10).
Ensure the rest of the checks (returncode and stderr handling) remain unchanged.
| def check_node_npm() -> tuple[bool, str]: # (is_available, error_message) | ||
| """ | ||
| Check if Node.js and npm are available. | ||
| If not available, attempts to install nvm and Node.js automatically. | ||
| """ | ||
|
|
||
| try: | ||
| # Check Node.js - try direct command first, then with nvm if needed | ||
| result = subprocess.run(["node", "--version"], capture_output=True, text=True, timeout=10) | ||
| if result.returncode != 0: | ||
| # If direct command fails, try with nvm sourced (in case nvm is installed but not in PATH) | ||
| nvm_path = get_nvm_sh_path() | ||
| if nvm_path.exists(): | ||
| result = subprocess.run( | ||
| ["bash", "-c", f"source {nvm_path} && node --version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
| if result.returncode != 0 and result.stderr: | ||
| logger.debug(f"Failed to source nvm or run node: {result.stderr.strip()}") | ||
| if result.returncode != 0: | ||
| # Node.js is not installed, try to install it | ||
| logger.info("Node.js is not installed. Attempting to install automatically...") | ||
|
|
||
| # Check if nvm is installed | ||
| if not check_nvm_installed(): | ||
| logger.info("nvm is not installed. Installing nvm first...") | ||
| if not install_nvm(): | ||
| return ( | ||
| False, | ||
| "Failed to install nvm. Please install Node.js manually from https://nodejs.org/", | ||
| ) | ||
|
|
||
| # Install Node.js using nvm | ||
| if not install_node_with_nvm(): | ||
| return ( | ||
| False, | ||
| "Failed to install Node.js. Please install Node.js manually from https://nodejs.org/", | ||
| ) | ||
|
|
||
| # Verify installation after automatic setup | ||
| # Try with nvm sourced first | ||
| nvm_path = get_nvm_sh_path() | ||
| if nvm_path.exists(): | ||
| result = subprocess.run( | ||
| ["bash", "-c", f"source {nvm_path} && node --version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
| if result.returncode != 0 and result.stderr: | ||
| logger.debug( | ||
| f"Failed to verify node after installation: {result.stderr.strip()}" | ||
| ) | ||
| else: | ||
| result = subprocess.run( | ||
| ["node", "--version"], capture_output=True, text=True, timeout=10 | ||
| ) | ||
| if result.returncode != 0: | ||
| nvm_path = get_nvm_sh_path() | ||
| return ( | ||
| False, | ||
| f"Node.js installation completed but node command is not available. Please restart your terminal or source {nvm_path}", | ||
| ) | ||
|
|
||
| node_version = result.stdout.strip() | ||
| logger.debug(f"Found Node.js version: {node_version}") | ||
|
|
||
| # Check npm - handle Windows PowerShell scripts | ||
| if platform.system() == "Windows": | ||
| # On Windows, npm might be a PowerShell script, so we need to use shell=True | ||
| result = subprocess.run( | ||
| ["npm", "--version"], capture_output=True, text=True, timeout=10, shell=True | ||
| ) | ||
| else: | ||
| # On Unix-like systems, if we just installed via nvm, we may need to source nvm | ||
| # Try direct command first | ||
| result = subprocess.run( | ||
| ["npm", "--version"], capture_output=True, text=True, timeout=10 | ||
| ) | ||
| if result.returncode != 0: | ||
| # Try with nvm sourced | ||
| nvm_path = get_nvm_sh_path() | ||
| if nvm_path.exists(): | ||
| result = subprocess.run( | ||
| ["bash", "-c", f"source {nvm_path} && npm --version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
| if result.returncode != 0 and result.stderr: | ||
| logger.debug(f"Failed to source nvm or run npm: {result.stderr.strip()}") | ||
|
|
||
| if result.returncode != 0: | ||
| return False, "npm is not installed or not in PATH" | ||
|
|
||
| npm_version = result.stdout.strip() | ||
| logger.debug(f"Found npm version: {npm_version}") | ||
|
|
||
| return True, f"Node.js {node_version}, npm {npm_version}" | ||
|
|
||
| except subprocess.TimeoutExpired: | ||
| return False, "Timeout checking Node.js/npm installation" | ||
| except FileNotFoundError: | ||
| # Node.js is not installed, try to install it | ||
| logger.info("Node.js is not found. Attempting to install automatically...") | ||
|
|
||
| # Check if nvm is installed | ||
| if not check_nvm_installed(): | ||
| logger.info("nvm is not installed. Installing nvm first...") | ||
| if not install_nvm(): | ||
| return ( | ||
| False, | ||
| "Failed to install nvm. Please install Node.js manually from https://nodejs.org/", | ||
| ) | ||
|
|
||
| # Install Node.js using nvm | ||
| if not install_node_with_nvm(): | ||
| return ( | ||
| False, | ||
| "Failed to install Node.js. Please install Node.js manually from https://nodejs.org/", | ||
| ) | ||
|
|
||
| # Retry checking Node.js after installation | ||
| try: | ||
| result = subprocess.run( | ||
| ["node", "--version"], capture_output=True, text=True, timeout=10 | ||
| ) | ||
| if result.returncode == 0: | ||
| node_version = result.stdout.strip() | ||
| # Check npm | ||
| nvm_path = get_nvm_sh_path() | ||
| if nvm_path.exists(): | ||
| result = subprocess.run( | ||
| ["bash", "-c", f"source {nvm_path} && npm --version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
| if result.returncode == 0: | ||
| npm_version = result.stdout.strip() | ||
| return True, f"Node.js {node_version}, npm {npm_version}" | ||
| elif result.stderr: | ||
| logger.debug(f"Failed to source nvm or run npm: {result.stderr.strip()}") | ||
| except Exception as e: | ||
| logger.debug(f"Exception retrying node/npm check: {str(e)}") | ||
|
|
||
| return False, "Node.js/npm not found. Please install Node.js from https://nodejs.org/" | ||
| except Exception as e: | ||
| return False, f"Error checking Node.js/npm: {str(e)}" |
There was a problem hiding this comment.
Fix subprocess argument handling and shell command path quoting in check_node_npm()
Two critical issues in the npm and nvm path handling:
-
Windows npm check (around line 260): Using a list with
shell=Truedrops arguments. Change:result = subprocess.run( ["npm", "--version"], capture_output=True, text=True, timeout=10, shell=True, )
to use a string instead:
result = subprocess.run( "npm --version", capture_output=True, text=True, timeout=10, shell=True, )
-
Unquoted nvm.sh paths in shell commands: All occurrences of
f"source {nvm_path} && ..."assume no spaces in the path. Quote the path to handle non-standard XDG/home directories:cmd = f'source "{nvm_path}" && node --version'
Apply the same quoting pattern to all similar shell command constructions (nvm + npm checks).
Optional: Factor the repeated "ensure nvm + Node installed" logic (appearing in both main try block and FileNotFoundError handler) into a helper function to reduce duplication.
🤖 Prompt for AI Agents
In cognee/api/v1/ui/node_setup.py around lines 210 to 360, fix two subprocess
invocation bugs: when calling subprocess.run(..., shell=True) on Windows pass a
single string command (e.g. "npm --version") instead of a list so arguments are
not dropped, and wrap any inserted nvm_sh path in quotes in the bash -c commands
(e.g. source "{nvm_path}" && ...) to handle paths with spaces; update all
occurrences of f"source {nvm_path} && ..." to use quoted paths and change the
Windows npm subprocess.run call to use a string command with shell=True;
optionally extract the repeated "ensure nvm + node installed" sequence into a
helper to remove duplication.
Description
Type of Change
Screenshots/Videos (if applicable)
Pre-submission Checklist
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.