Skip to content

chore(structure): Remove redundant projectRoot arg#1386

Merged
Tobbe merged 9 commits intomainfrom
tobbe-chore-structure-remove-project-root
Mar 17, 2026
Merged

chore(structure): Remove redundant projectRoot arg#1386
Tobbe merged 9 commits intomainfrom
tobbe-chore-structure-remove-project-root

Conversation

@Tobbe
Copy link
Member

@Tobbe Tobbe commented Mar 17, 2026

We only ever passed getPaths().base as the argument, which is exactly what the default is. So no need to keep it.

This also simplified the implementation for #1337 because it can also rely on just the default .base dir

@netlify
Copy link

netlify bot commented Mar 17, 2026

Deploy Preview for cedarjs canceled.

Name Link
🔨 Latest commit 729079e
🔍 Latest deploy log https://app.netlify.com/projects/cedarjs/deploys/69b964cd0a3df200074314ec

@github-actions github-actions bot added this to the chore milestone Mar 17, 2026
'--root',
getPaths().base,
]
const scriptArgs = [path.join(__dirname, 'scripts', 'invoke.js'), ...args]

Check warning

Code scanning / CodeQL

Unsafe shell command constructed from library input Medium

This shell argument which depends on
library input
is later used in a
shell command
.
This shell argument which depends on
library input
is later used in a
shell command
.
This shell argument which depends on
library input
is later used in a
shell command
.
This shell argument which depends on
library input
is later used in a
shell command
.
This shell argument which depends on
library input
is later used in a
shell command
.

Copilot Autofix

AI 1 day ago

In general, the fix is to avoid passing untrusted or library-controlled input through the shell. With Node’s child_process.spawn, this means not combining user-controlled strings into a single command string when shell: true. Instead, either (a) disable shell: true and pass a command plus an array of arguments, or (b) if shell: true is truly required (for pipes, redirects, etc.), then robustly escape any dynamic segments before interpolation.

Here, we don’t actually need shell features: we just want to execute node <invoke.js> ...args. The clean fix is:

  • Use the same safe spawn(process.execPath, scriptArgs, spawnOptions) call path on all platforms, including Windows.
  • Remove shell: true from the Windows spawnOptions. That eliminates shell interpretation entirely, making special characters in telemetry payloads harmless.
  • Keep the rest of the behavior (stdio, detached/windowsHide flags) unchanged as far as possible.

Concretely, in packages/telemetry/src/telemetry.ts:

  1. In the Windows branch of spawnOptions, set shell: false (or just remove the property; by default it is false).
  2. Remove the special Windows handling that builds a command string:
    • Delete const execPath = ....
    • Remove the if (isWindows) { ... } else { ... } conditional.
    • Replace it with a single spawn(process.execPath, scriptArgs, spawnOptions).unref() call that runs on all platforms.

No other files need modification, since the issue arises only from this use of spawn with shell: true and a concatenated command string.


Suggested changeset 1
packages/telemetry/src/telemetry.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/packages/telemetry/src/telemetry.ts b/packages/telemetry/src/telemetry.ts
--- a/packages/telemetry/src/telemetry.ts
+++ b/packages/telemetry/src/telemetry.ts
@@ -7,7 +7,6 @@
   // "os.type()" returns 'Windows_NT' on Windows.
   // See https://nodejs.org/docs/latest-v12.x/api/os.html#os_os_type.
   const isWindows = os.type() === 'Windows_NT'
-  const execPath = isWindows ? `"${process.execPath}"` : process.execPath
 
   const spawnOptions: Partial<SpawnOptions> = isWindows
     ? {
@@ -18,7 +17,6 @@
         // See https://github.com/nodejs/node/issues/21825#issuecomment-503766781 for information
         detached: false,
         windowsHide: false,
-        shell: true,
       }
     : {
         stdio: process.env.REDWOOD_VERBOSE_TELEMETRY
@@ -30,14 +28,8 @@
 
   const scriptArgs = [path.join(__dirname, 'scripts', 'invoke.js'), ...args]
 
-  if (isWindows) {
-    // Use command string with empty args array to avoid DEP0190 warning when
-    // `shell: true`
-    spawn([execPath, ...scriptArgs].join(' '), [], spawnOptions).unref()
-  } else {
-    // Use proper args array when no shell needed
-    spawn(process.execPath, scriptArgs, spawnOptions).unref()
-  }
+  // Use proper args array so no shell interpretation occurs on any platform
+  spawn(process.execPath, scriptArgs, spawnOptions).unref()
 }
 
 // wrap a function in this call to get a telemetry hit including how long it took
EOF
@@ -7,7 +7,6 @@
// "os.type()" returns 'Windows_NT' on Windows.
// See https://nodejs.org/docs/latest-v12.x/api/os.html#os_os_type.
const isWindows = os.type() === 'Windows_NT'
const execPath = isWindows ? `"${process.execPath}"` : process.execPath

const spawnOptions: Partial<SpawnOptions> = isWindows
? {
@@ -18,7 +17,6 @@
// See https://github.com/nodejs/node/issues/21825#issuecomment-503766781 for information
detached: false,
windowsHide: false,
shell: true,
}
: {
stdio: process.env.REDWOOD_VERBOSE_TELEMETRY
@@ -30,14 +28,8 @@

const scriptArgs = [path.join(__dirname, 'scripts', 'invoke.js'), ...args]

if (isWindows) {
// Use command string with empty args array to avoid DEP0190 warning when
// `shell: true`
spawn([execPath, ...scriptArgs].join(' '), [], spawnOptions).unref()
} else {
// Use proper args array when no shell needed
spawn(process.execPath, scriptArgs, spawnOptions).unref()
}
// Use proper args array so no shell interpretation occurs on any platform
spawn(process.execPath, scriptArgs, spawnOptions).unref()
}

// wrap a function in this call to get a telemetry hit including how long it took
Copilot is powered by AI and may make mistakes. Always verify output.
@nx-cloud
Copy link

nx-cloud bot commented Mar 17, 2026

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 729079e

Command Status Duration Result
nx run-many -t build:pack --exclude create-ceda... ✅ Succeeded 8s View ↗
nx run-many -t test --minWorkers=1 --maxWorkers=4 ✅ Succeeded 1m 35s View ↗
nx run-many -t build ✅ Succeeded 7s View ↗
nx run-many -t test:types ✅ Succeeded 11s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-17 14:44:46 UTC

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 17, 2026

Greptile Summary

This PR removes the redundant projectRoot argument that was being passed through to getProject(), RWProject, and related APIs across packages/structure, packages/internal, packages/prerender, packages/vite, and packages/telemetry. All callers were simply passing getPaths().base, so the argument was collapsed into the internal getPaths() call inside RWProject.pathHelper. The refactor is well-scoped and correct in every changed file — however, packages/cli/src/commands/check.ts was not updated and now calls printDiagnostics with the old two-argument signature, silently dropping the getSeverityLabel formatting callback.

Key changes:

  • RWProject no longer accepts a constructor argument; it resolves the project root via getPaths() internally
  • getProject() and printDiagnostics() in packages/structure/src/index.ts drop their projectRoot parameter
  • Telemetry subprocess no longer passes --root to the child process; sendTelemetry.ts calls getPaths() directly
  • Tests updated to set process.env.CEDAR_CWD before constructing RWProject instead of passing the path as a constructor argument
  • packages/cli/src/commands/check.ts:22 still calls printDiagnostics(getPaths().base, { getSeverityLabel }) — the path string lands in the opts slot and the callback object is silently ignored, breaking severity label formatting in the check command

Confidence Score: 3/5

  • Mostly safe, but packages/cli/src/commands/check.ts was not updated and will silently misbehave after this change.
  • All ten changed files are correctly refactored and the simplification is sound. The one issue is a missed update in an unchanged file (check.ts) whose printDiagnostics call now passes arguments in the wrong positions — the path string becomes opts and the real options object is dropped, causing severity labels to be lost in the check / diagnostics command.
  • packages/cli/src/commands/check.ts — call to printDiagnostics must be updated to match the new one-argument signature

Important Files Changed

Filename Overview
packages/structure/src/index.ts Removes projectRoot param from getProject() and printDiagnostics(), relying on getPaths() internally. The signature change for printDiagnostics creates a mismatch with its CLI caller.
packages/cli/src/commands/check.ts Not changed in this PR, but now broken: still calls printDiagnostics(getPaths().base, { getSeverityLabel }) with the old two-arg signature, causing the getSeverityLabel callback to be silently dropped.
packages/structure/src/model/RWProject.ts Removes RWProjectOptions interface and projectRoot getter; pathHelper now calls getPaths() directly. Clean simplification with no functional regression.
packages/telemetry/src/sendTelemetry.ts Removes argv.root / --root plumbing; project stats are now collected unconditionally via new RWProject() and getPaths(). Matches the old behavior since rootDir was always truthy in normal flows.
packages/structure/src/model/tests/model.test.ts Tests updated to set CEDAR_CWD env var before constructing new RWProject() instead of passing projectRoot directly; correctly saves/restores the env variable.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Callers\n(routes.ts, entries.ts,\ndetection.ts, sendTelemetry.ts)"] -->|"getProject()\nno args"| B["getProject()\npackages/structure/src/index.ts"]
    B --> C["new RWProject()"]
    C --> D["RWProject.pathHelper\n@lazy getPaths()"]
    D --> E["CEDAR_CWD env var\n/ process.cwd()"]

    F["CLI check.ts"] -->|"printDiagnostics(getPaths().base, opts)\nOLD signature ❌"| G["printDiagnostics(opts?)\nNEW signature"]
    G --> D

    style F fill:#ff9999
    style G fill:#ffe599
Loading

Comments Outside Diff (1)

  1. packages/cli/src/commands/check.ts, line 22-34 (link)

    Caller not updated for new printDiagnostics signature

    printDiagnostics was changed from (projectRoot: string, opts?) to (opts?) in this PR, but this call site was not updated. As written, getPaths().base (a string) is forwarded as the opts argument, and the actual options object containing getSeverityLabel is passed as a second argument that the new signature silently ignores.

    The practical effect is that the custom severity label formatting (c.error, c.warning, c.info) is never applied — getSeverityLabel will be undefined inside printDiagnostics.

    The call should be updated to drop the first argument:

Last reviewed commit: ecce596

@Tobbe
Copy link
Member Author

Tobbe commented Mar 17, 2026

@greptileai please do a full review again

@Tobbe
Copy link
Member Author

Tobbe commented Mar 17, 2026

@greptileai do a full review again please

@Tobbe Tobbe merged commit 242e0ff into main Mar 17, 2026
41 checks passed
@Tobbe Tobbe deleted the tobbe-chore-structure-remove-project-root branch March 17, 2026 14:52
@github-actions
Copy link

The changes in this PR are now available on npm.

Try them out by running yarn cedar upgrade -t 3.0.0-canary.13595

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant