diff --git a/artifactory/commands/pnpm/install.go b/artifactory/commands/pnpm/install.go index 9cf7a73e..2cb3432a 100644 --- a/artifactory/commands/pnpm/install.go +++ b/artifactory/commands/pnpm/install.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "os/exec" + "path/filepath" + "strconv" "strings" "github.com/jfrog/build-info-go/build" @@ -100,8 +102,13 @@ func (pic *PnpmInstallCommand) collectAndSaveBuildInfo() error { } log.Debug("Server details. Artifactory URL:", pic.serverDetails.ArtifactoryUrl) + scopeToCurrentPackage := isPnpmWorkspaceSubPackage(pic.workingDirectory) + if scopeToCurrentPackage && userRequestedWorkspaceRoot(pic.pnpmArgs) { + log.Debug("--workspace-root/-w specified; collecting build-info for the entire workspace despite sub-package cwd.") + scopeToCurrentPackage = false + } log.Debug("Running pnpm ls to collect dependency tree...") - projects, err := runPnpmLs(pic.workingDirectory) + projects, err := runPnpmLs(pic.workingDirectory, extractLsForwardFlags(pic.pnpmArgs), scopeToCurrentPackage) if err != nil { return err } @@ -114,8 +121,185 @@ func (pic *PnpmInstallCommand) collectAndSaveBuildInfo() error { return saveBuildInfo(modules, pic.buildConfiguration) } -func runPnpmLs(workingDir string) ([]pnpmLsProject, error) { - pnpmLsArgs := []string{"ls", "-r", "--depth", "Infinity", "--json"} +// isPnpmWorkspaceSubPackage returns true when workingDir is inside a pnpm workspace +// but is NOT the workspace root (i.e. the user invoked the command from a single +// workspace package). When true, build-info should be scoped to that package only. +// +// Detection is delegated to pnpm itself via `pnpm root -w`, which prints the +// workspace root's node_modules path when inside a workspace and errors with a +// non-zero exit code otherwise. Any error (not a workspace, pnpm missing, etc.) +// is logged at debug level and treated as "not a sub-package" — in that case +// the caller keeps the existing recursive behavior. +func isPnpmWorkspaceSubPackage(workingDir string) bool { + cmd := exec.Command("pnpm", "root", "-w") + cmd.Dir = workingDir + // Split stdout/stderr so future pnpm warnings/deprecations on stderr can't + // corrupt the path we parse from stdout. Both streams are surfaced in the + // debug log on failure to preserve diagnostic coverage when the real error + // reason can be on either stream. + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + log.Debug(fmt.Sprintf("pnpm workspace detection skipped (%s; stdout: %q; stderr: %q); falling back to recursive pnpm ls.", + err.Error(), + strings.TrimSpace(stdout.String()), + strings.TrimSpace(stderr.String()))) + return false + } + workspaceRoot := filepath.Dir(strings.TrimSpace(stdout.String())) + // filepath.Dir collapses "" / single-segment input to ".", so "." is the + // only "no usable path" sentinel we need to guard against. + if workspaceRoot == "." { + log.Debug("pnpm root -w returned no usable path; falling back to recursive pnpm ls.") + return false + } + // pnpm resolves symlinks in its output (e.g. on macOS /var → /private/var), so + // compare canonicalized paths to avoid false "sub-package" detection when the + // working directory happens to contain a symlink. + if samePath(workspaceRoot, workingDir) { + log.Debug("Invoked at pnpm workspace root; collecting build-info for all workspace packages.") + return false + } + log.Debug(fmt.Sprintf("Invoked inside workspace sub-package (workspace root: %s); scoping build-info to current package.", workspaceRoot)) + return true +} + +// samePath reports whether a and b refer to the same directory after resolving +// symlinks. If symlink resolution fails on either side (e.g. one path no longer +// exists on disk), it falls back to direct string equality — i.e. it returns +// false unless a == b verbatim. No further inference is attempted. +func samePath(a, b string) bool { + if a == b { + return true + } + resolvedA, errA := filepath.EvalSymlinks(a) + resolvedB, errB := filepath.EvalSymlinks(b) + if errA != nil || errB != nil { + return false + } + return resolvedA == resolvedB +} + +// lsForwardFlags maps `pnpm install` flags that affect the resolved dependency +// tree to whether the flag carries a value. Boolean flags (false) are forwarded +// as-is. Value-bearing flags (true) accept either `--flag value` or `--flag=value`. +// Anything not in this map is dropped — install-only flags (e.g. --frozen-lockfile) +// would just confuse `pnpm ls`. +// +// Notably absent: --no-optional. The build-info parser currently does not read +// pnpm ls's `optionalDependencies` JSON key (only `dependencies` and +// `devDependencies`), so forwarding --no-optional has no observable effect on +// build-info. Tracked as a follow-up to extend the parser; once that lands, +// --no-optional should be added back here. +var lsForwardFlags = map[string]bool{ + "--ignore-workspace": false, + "--prod": false, + "-P": false, + "--production": false, + "--dev": false, + "-D": false, + "--workspace-root": false, + "-w": false, + "--filter": true, + "-F": true, +} + +// userRequestedWorkspaceRoot reports whether the user asked pnpm install to +// operate at the workspace root (via --workspace-root or its alias -w). When +// true, build-info must reflect the entire workspace even if invoked from a +// sub-package, so the cwd-based scope heuristic must be overridden. Conflict +// resolution between this and other flags (e.g. --ignore-workspace) is left +// to pnpm itself via the forwarded flags. +func userRequestedWorkspaceRoot(installArgs []string) bool { + for _, arg := range installArgs { + if arg == "--workspace-root" || arg == "-w" { + return true + } + } + return false +} + +// extractLsForwardFlags returns flags from the user's pnpm install args that must +// be forwarded to `pnpm ls` so the dependency tree respects the same scope and +// dep-group filtering as the install command. Both `--flag=value` and +// `--flag value` forms are recognized for value-bearing flags. +func extractLsForwardFlags(installArgs []string) []string { + var forwarded []string + for i := 0; i < len(installArgs); i++ { + arg := installArgs[i] + + if takesValue, ok := lsForwardFlags[arg]; ok { + if !takesValue { + forwarded = append(forwarded, arg) + continue + } + // `--flag value` form — capture the next arg as the value when present. + if i+1 < len(installArgs) { + forwarded = append(forwarded, arg, installArgs[i+1]) + i++ + } + continue + } + + // `--flag=value` form — split on the first '=' and check the prefix. + if eq := strings.IndexByte(arg, '='); eq > 0 { + key, val := arg[:eq], arg[eq+1:] + if takesValue, ok := lsForwardFlags[key]; ok { + // Value-bearing flags (e.g. --filter=foo) always forward as-is. + if takesValue { + forwarded = append(forwarded, arg) + continue + } + // Boolean flags with =value (e.g. --prod=true, --ignore-workspace=0): + // drop only when strconv.ParseBool recognizes a falsy value + // (false / 0 / F). Anything unparseable is forwarded as-is so pnpm + // can decide. This matches jfrog-cli-core's FindBooleanFlag semantics. + if parsed, err := strconv.ParseBool(val); err != nil || parsed { + forwarded = append(forwarded, arg) + } + } + } + } + return forwarded +} + +// buildPnpmLsArgs assembles the argument list for `pnpm ls`. The `-r` flag is omitted +// when either (a) we are scoping output to the current working directory's package, or +// (b) the user forwarded --ignore-workspace — in that case pnpm emits one JSON array +// per project concatenated on stdout, which is not parseable as a single array. +func buildPnpmLsArgs(extraArgs []string, scopeToCurrentPackage bool) []string { + args := []string{"ls"} + if !scopeToCurrentPackage && !hasIgnoreWorkspace(extraArgs) { + args = append(args, "-r") + } + args = append(args, "--depth", "Infinity", "--json") + args = append(args, extraArgs...) + return args +} + +// hasIgnoreWorkspace reports whether `--ignore-workspace` is present in args, +// matching both the bare flag and the `--ignore-workspace=` form. The +// value is parsed via strconv.ParseBool to match jfrog-cli-core's bool flag +// semantics; an unparseable value is conservatively treated as enabled and +// left to pnpm to validate. +func hasIgnoreWorkspace(args []string) bool { + const prefix = "--ignore-workspace=" + for _, a := range args { + if a == "--ignore-workspace" { + return true + } + if strings.HasPrefix(a, prefix) { + if parsed, err := strconv.ParseBool(a[len(prefix):]); err != nil || parsed { + return true + } + } + } + return false +} + +func runPnpmLs(workingDir string, extraArgs []string, scopeToCurrentPackage bool) ([]pnpmLsProject, error) { + pnpmLsArgs := buildPnpmLsArgs(extraArgs, scopeToCurrentPackage) log.Info("Collecting dependency tree information. This may take a few minutes for large projects...") log.Debug("Running command: pnpm", strings.Join(pnpmLsArgs, " ")) command := exec.Command("pnpm", pnpmLsArgs...) diff --git a/artifactory/commands/pnpm/pnpm_test.go b/artifactory/commands/pnpm/pnpm_test.go index 32c19da9..84e56899 100644 --- a/artifactory/commands/pnpm/pnpm_test.go +++ b/artifactory/commands/pnpm/pnpm_test.go @@ -1,12 +1,15 @@ package pnpm import ( + "os" + "path/filepath" "testing" "github.com/jfrog/build-info-go/entities" "github.com/jfrog/gofrog/version" servicesUtils "github.com/jfrog/jfrog-client-go/artifactory/services/utils" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestResolveRepoFromRegistry(t *testing.T) { @@ -420,6 +423,251 @@ func TestNewCommandUnsupported(t *testing.T) { assert.Contains(t, err.Error(), "unsupported pnpm command") } +// TestUserRequestedWorkspaceRoot verifies detection of the --workspace-root / -w +// flag, which signals the user wants pnpm install to operate at the workspace +// root and therefore build-info must NOT be scoped to a sub-package. +func TestUserRequestedWorkspaceRoot(t *testing.T) { + tests := []struct { + name string + args []string + want bool + }{ + {"no flags", nil, false}, + {"unrelated flags", []string{"--frozen-lockfile", "--prod"}, false}, + {"long form alone", []string{"--workspace-root"}, true}, + {"short form alone", []string{"-w"}, true}, + {"long form mixed", []string{"--prod", "--workspace-root", "--frozen-lockfile"}, true}, + {"short form mixed", []string{"--prod", "-w", "--frozen-lockfile"}, true}, + // -W is not the same flag — must not match. + {"capital -W is not workspace-root", []string{"-W"}, false}, + // Substring of an unrelated flag must not match. + {"longer flag containing 'workspace-root' substring", []string{"--no-workspace-root"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, userRequestedWorkspaceRoot(tt.args)) + }) + } +} + +// TestExtractLsForwardFlags verifies that flags from `pnpm install` that affect +// the resolved dependency tree (workspace scope and dep-group filtering) are +// forwarded to `pnpm ls`, while install-only flags are dropped. +func TestExtractLsForwardFlags(t *testing.T) { + tests := []struct { + name string + args []string + want []string + }{ + {"no flags", []string{}, nil}, + {"only install-only flags", []string{"--frozen-lockfile", "--no-color"}, nil}, + {"ignore-workspace", []string{"--ignore-workspace"}, []string{"--ignore-workspace"}}, + {"prod", []string{"--prod"}, []string{"--prod"}}, + {"production", []string{"--production"}, []string{"--production"}}, + {"dev", []string{"--dev"}, []string{"--dev"}}, + // --no-optional is intentionally NOT forwarded today: the build-info + // parser doesn't read pnpm ls's `optionalDependencies` JSON key, so the + // flag would be a no-op. Locked here so a future re-add is intentional. + {"no-optional dropped (parser does not read optionalDeps)", []string{"--no-optional"}, nil}, + {"workspace-root", []string{"--workspace-root"}, []string{"--workspace-root"}}, + { + name: "filter with separate value", + args: []string{"--filter", "web-app"}, + want: []string{"--filter", "web-app"}, + }, + { + name: "filter with attached value", + args: []string{"--filter=web-app"}, + want: []string{"--filter=web-app"}, + }, + { + name: "repeated filter", + args: []string{"--filter=a", "--filter", "b"}, + want: []string{"--filter=a", "--filter", "b"}, + }, + { + name: "all forwarded flags mixed with install-only", + args: []string{"--frozen-lockfile", "--ignore-workspace", "--prod", "--no-optional", "--filter", "pkg", "--reporter=ndjson"}, + want: []string{"--ignore-workspace", "--prod", "--filter", "pkg"}, + }, + { + // Defensive: `--filter` at the very end with no value should be dropped rather + // than capturing the (non-existent) next arg or emitting a dangling flag. + name: "filter without value at end is dropped", + args: []string{"--ignore-workspace", "--filter"}, + want: []string{"--ignore-workspace"}, + }, + // Short-form aliases supported by pnpm install: -P (--prod), -D (--dev), + // -F (--filter), -w (--workspace-root). All have the same semantics in + // pnpm ls and must be forwarded so build-info matches the install scope. + {"short-form -P forwarded", []string{"-P"}, []string{"-P"}}, + {"short-form -D forwarded", []string{"-D"}, []string{"-D"}}, + {"short-form -w forwarded", []string{"-w"}, []string{"-w"}}, + {"short-form -F with separate value", []string{"-F", "web-app"}, []string{"-F", "web-app"}}, + {"short-form -F with attached value", []string{"-F=web-app"}, []string{"-F=web-app"}}, + {"short-form -F without value at end is dropped", []string{"-F"}, nil}, + { + // Unrelated `--foo=bar` style flags should not be forwarded just because + // they contain '='. + name: "unrelated --flag=value not forwarded", + args: []string{"--reporter=ndjson"}, + want: nil, + }, + // Boolean flags with `=value` form. Falsy values (false, 0, F) are dropped; + // truthy values (true, 1, T) are forwarded; unparseable values are forwarded + // so pnpm can validate. Matches strconv.ParseBool / jfrog-cli-core semantics. + {"bool =true forwarded", []string{"--ignore-workspace=true"}, []string{"--ignore-workspace=true"}}, + {"bool =false dropped", []string{"--ignore-workspace=false"}, nil}, + {"bool =1 forwarded", []string{"--prod=1"}, []string{"--prod=1"}}, + {"bool =0 dropped", []string{"--prod=0"}, nil}, + {"bool =T forwarded", []string{"--prod=T"}, []string{"--prod=T"}}, + {"bool =F dropped", []string{"--prod=F"}, nil}, + {"bool =empty forwarded (unparseable, deferred to pnpm)", []string{"--ignore-workspace="}, []string{"--ignore-workspace="}}, + {"bool =garbage forwarded (unparseable, deferred to pnpm)", []string{"--prod=maybe"}, []string{"--prod=maybe"}}, + { + name: "mixed bool =value with bare and value flags", + args: []string{"--prod=true", "--dev", "--filter=pkg"}, + want: []string{"--prod=true", "--dev", "--filter=pkg"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, extractLsForwardFlags(tt.args)) + }) + } +} + +// TestBuildPnpmLsArgs verifies the pnpm ls command line is assembled correctly +// based on workspace context and forwarded flags. +func TestBuildPnpmLsArgs(t *testing.T) { + tests := []struct { + name string + extraArgs []string + scopeToCurrentPackage bool + want []string + }{ + { + name: "workspace root (no scoping, no extra flags)", + extraArgs: nil, + scopeToCurrentPackage: false, + want: []string{"ls", "-r", "--depth", "Infinity", "--json"}, + }, + { + name: "sub-package (scoped, no extra flags)", + extraArgs: nil, + scopeToCurrentPackage: true, + want: []string{"ls", "--depth", "Infinity", "--json"}, + }, + { + // `pnpm ls -r --ignore-workspace` prints one JSON array per project concatenated + // on stdout, which is not parseable. When --ignore-workspace is forwarded we must + // drop -r regardless of cwd so the output stays a single JSON array. + name: "workspace root with ignore-workspace forwarded (must drop -r)", + extraArgs: []string{"--ignore-workspace"}, + scopeToCurrentPackage: false, + want: []string{"ls", "--depth", "Infinity", "--json", "--ignore-workspace"}, + }, + { + name: "sub-package with ignore-workspace forwarded", + extraArgs: []string{"--ignore-workspace"}, + scopeToCurrentPackage: true, + want: []string{"ls", "--depth", "Infinity", "--json", "--ignore-workspace"}, + }, + { + // `--ignore-workspace=true` must drop -r the same way the bare flag does, + // otherwise pnpm emits unparseable concatenated JSON. + name: "workspace root with --ignore-workspace=true (must drop -r)", + extraArgs: []string{"--ignore-workspace=true"}, + scopeToCurrentPackage: false, + want: []string{"ls", "--depth", "Infinity", "--json", "--ignore-workspace=true"}, + }, + { + // `--ignore-workspace=false` is a no-op — `-r` must still be added. + name: "workspace root with --ignore-workspace=false (keeps -r)", + extraArgs: []string{"--ignore-workspace=false"}, + scopeToCurrentPackage: false, + want: []string{"ls", "-r", "--depth", "Infinity", "--json", "--ignore-workspace=false"}, + }, + { + // `--ignore-workspace=0` is also falsy (strconv.ParseBool) — keep -r. + name: "workspace root with --ignore-workspace=0 (keeps -r)", + extraArgs: []string{"--ignore-workspace=0"}, + scopeToCurrentPackage: false, + want: []string{"ls", "-r", "--depth", "Infinity", "--json", "--ignore-workspace=0"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, buildPnpmLsArgs(tt.extraArgs, tt.scopeToCurrentPackage)) + }) + } +} + +// TestSamePath verifies that symlinked paths are recognized as the same directory. +// This matters on macOS where /var is a symlink to /private/var, and pnpm resolves +// symlinks in its output while our workingDir is typically unresolved. +func TestSamePath(t *testing.T) { + t.Run("identical strings", func(t *testing.T) { + assert.True(t, samePath("/foo/bar", "/foo/bar")) + }) + + t.Run("different strings", func(t *testing.T) { + assert.False(t, samePath("/foo/bar", "/foo/baz")) + }) + + t.Run("symlinked directories resolve equal", func(t *testing.T) { + real := t.TempDir() + linkDir := filepath.Join(t.TempDir(), "link") + require.NoError(t, os.Symlink(real, linkDir)) + + assert.True(t, samePath(real, linkDir), + "paths pointing to the same directory via a symlink should be treated as equal") + }) + + t.Run("non-existent paths fall back to string comparison", func(t *testing.T) { + assert.False(t, samePath("/does/not/exist/a", "/does/not/exist/b")) + assert.True(t, samePath("/does/not/exist", "/does/not/exist")) + }) +} + +// TestIsPnpmWorkspaceSubPackage exercises the detection helper against three +// realistic filesystem layouts. It shells out to pnpm (available in CI per +// TestValidatePnpmPrerequisites) but does not require any packages to be installed. +func TestIsPnpmWorkspaceSubPackage(t *testing.T) { + t.Run("non-workspace directory returns false", func(t *testing.T) { + dir := t.TempDir() + assert.False(t, isPnpmWorkspaceSubPackage(dir), + "a plain directory with no pnpm-workspace.yaml must not be treated as a sub-package") + }) + + t.Run("workspace root returns false", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "pnpm-workspace.yaml"), + []byte("packages:\n - 'apps/*'\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(root, "package.json"), + []byte(`{"name":"root","version":"1.0.0"}`), 0o644)) + + assert.False(t, isPnpmWorkspaceSubPackage(root), + "workingDir equal to the workspace root must not be treated as a sub-package") + }) + + t.Run("workspace sub-package returns true", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "pnpm-workspace.yaml"), + []byte("packages:\n - 'apps/*'\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(root, "package.json"), + []byte(`{"name":"root","version":"1.0.0"}`), 0o644)) + + subPkg := filepath.Join(root, "apps", "web-app") + require.NoError(t, os.MkdirAll(subPkg, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(subPkg, "package.json"), + []byte(`{"name":"web-app","version":"1.0.0"}`), 0o644)) + + assert.True(t, isPnpmWorkspaceSubPackage(subPkg), + "workingDir inside a workspace package must be treated as a sub-package") + }) +} + // TestParsePnpmLsProjectsEmpty verifies handling of empty/minimal pnpm ls output (RTECO-903). func TestParsePnpmLsProjectsEmpty(t *testing.T) { mods := parsePnpmLsProjects([]pnpmLsProject{})