From b2b1cf65b0067dfdaea29f3285abd9b42aeae787 Mon Sep 17 00:00:00 2001 From: Ellen Agarwal Date: Fri, 10 Jul 2026 17:12:17 -0700 Subject: [PATCH] fix(cli): accept top-level --if-present and stop workspace installs scaffolding a root manifest (#12919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accept --if-present ahead of the script name, the way pnpm's option parser does. The repo's own test-pkgs-branch script uses that spelling, so every PR TS CI job died at argument parsing under the pacquet-based pnpm v12. Declared top-level like the other run-scoped options but not clap-global (run/stop/restart declare their own --if-present), merged with the subcommand flags, validated like --resume-from/--no-bail. Rejected for exec like pnpm, where dispatch would drop it silently. Never scaffold a missing manifest inside a workspace. Gate State::init's no-scaffold path on workspace membership (config.workspace_dir) rather than a pnpm-workspace.yaml beside the missing manifest, so installs in workspace member directories can't scaffold a package.json either — pnpm never creates one anywhere in a workspace. Keep an empty object as the in-memory stand-in instead of the init template: pnpm's add at a rootless workspace root persists a manifest holding only the added dependencies, and the template's failing test script would otherwise ride along on any explicit save. Together with pnpm/pnpm#12914 this unblocks the ci:test-branch job once a pnpm v12 alpha carrying both is released and packageManager is bumped. --- pnpm/crates/cli/src/cli_args/cli_command.rs | 34 +++++++++++- pnpm/crates/cli/src/cli_args/dispatch.rs | 5 ++ .../cli/src/cli_args/dispatch_script.rs | 7 ++- pnpm/crates/cli/src/cli_args/tests.rs | 55 +++++++++++++++++++ pnpm/crates/cli/src/state.rs | 24 ++++---- pnpm/crates/cli/tests/run.rs | 20 +++++++ pnpm/crates/cli/tests/run_recursive.rs | 21 +++++++ 7 files changed, 151 insertions(+), 15 deletions(-) diff --git a/pnpm/crates/cli/src/cli_args/cli_command.rs b/pnpm/crates/cli/src/cli_args/cli_command.rs index e0aace20ce..6bf0dd4751 100644 --- a/pnpm/crates/cli/src/cli_args/cli_command.rs +++ b/pnpm/crates/cli/src/cli_args/cli_command.rs @@ -182,6 +182,15 @@ pub struct CliArgs { /// Recursive only: keep going after a project fails. #[clap(long = "no-bail", global = true, hide = true)] pub no_bail: bool, + + /// Avoid exiting with a non-zero exit code when the script is + /// undefined, accepted ahead of the script name + /// (`pnpm --if-present test`) the way pnpm's option parser does. + /// Deliberately not `global = true`: `run` / `stop` / `restart` + /// declare their own `--if-present`, and a propagated global flag + /// would collide with theirs. + #[clap(long = "if-present", hide = true)] + pub if_present: bool, } fn parse_store_dir(value: &str) -> Result { @@ -199,6 +208,9 @@ impl CliArgs { if self.no_bail { self.validate_run_scoped_global_option("--no-bail")?; } + if self.if_present { + self.validate_if_present_top_level_option()?; + } Ok(()) } @@ -216,6 +228,20 @@ impl CliArgs { } } + /// `restart` also runs scripts and accepts its own `--if-present`, + /// so the top-level spelling is valid for it too — unlike the + /// recursive-only flags, which `restart` rejects. `exec` is the + /// reverse: it takes the recursive-only flags but runs arbitrary + /// commands rather than scripts, so pnpm rejects `--if-present` + /// for it and pacquet must too. + fn validate_if_present_top_level_option(&self) -> Result<(), clap::Error> { + match self.command { + CliCommand::Restart(_) => Ok(()), + CliCommand::Exec(_) => Err(Self::unexpected_argument_error("--if-present")), + _ => self.validate_run_scoped_global_option("--if-present"), + } + } + fn validate_run_scoped_global_option(&self, option: &str) -> Result<(), clap::Error> { if matches!( self.command, @@ -228,8 +254,12 @@ impl CliArgs { ) { return Ok(()); } - Err(Self::command() - .error(ErrorKind::UnknownArgument, format!("unexpected argument '{option}' found"))) + Err(Self::unexpected_argument_error(option)) + } + + fn unexpected_argument_error(option: &str) -> clap::Error { + Self::command() + .error(ErrorKind::UnknownArgument, format!("unexpected argument '{option}' found")) } fn validate_report_summary_global_option(&self) -> Result<(), clap::Error> { diff --git a/pnpm/crates/cli/src/cli_args/dispatch.rs b/pnpm/crates/cli/src/cli_args/dispatch.rs index 1e18c901b4..6950caf128 100644 --- a/pnpm/crates/cli/src/cli_args/dispatch.rs +++ b/pnpm/crates/cli/src/cli_args/dispatch.rs @@ -36,6 +36,9 @@ pub(crate) struct RunCtx<'a> { pub(crate) recursive_report_summary: bool, pub(crate) recursive_no_bail: bool, pub(crate) recursive_sort: bool, + /// The top-level `--if-present` spelling (`pnpm --if-present test`); + /// merged with the flag the script subcommands declare themselves. + pub(crate) if_present: bool, pub(crate) config: &'a (dyn Fn() -> miette::Result<&'static mut Config> + Sync), /// Like [`Self::config`] but anchored at the pnpm home dir instead of /// `--dir`, so a `-g` install can't inherit the caller project's @@ -139,6 +142,7 @@ impl CliArgs { resume_from, report_summary, no_bail, + if_present, } = self; // Canonicalize `--dir` so the bunyan-envelope `prefix` emitted by @@ -259,6 +263,7 @@ impl CliArgs { recursive_report_summary: report_summary, recursive_no_bail: no_bail, recursive_sort: !no_sort, + if_present, config: &config, global_config: &global_config, state: &state, diff --git a/pnpm/crates/cli/src/cli_args/dispatch_script.rs b/pnpm/crates/cli/src/cli_args/dispatch_script.rs index 1daaf3a223..d57a0d1504 100644 --- a/pnpm/crates/cli/src/cli_args/dispatch_script.rs +++ b/pnpm/crates/cli/src/cli_args/dispatch_script.rs @@ -88,6 +88,7 @@ fn with_recursive_run_options(ctx: &RunCtx<'_>, mut args: RunArgs) -> RunArgs { args.report_summary = ctx.recursive_report_summary; args.no_bail = ctx.recursive_no_bail; args.sort = ctx.recursive_sort; + args.if_present |= ctx.if_present; args } @@ -103,7 +104,8 @@ pub(super) fn start<'a>(ctx: &RunCtx<'a>) -> miette::Result> { run(ctx, run_args_for_script("start", false)) } -pub(super) fn stop<'a>(ctx: &RunCtx<'a>, args: StopArgs) -> miette::Result> { +pub(super) fn stop<'a>(ctx: &RunCtx<'a>, mut args: StopArgs) -> miette::Result> { + args.if_present |= ctx.if_present; if ctx.recursive { run(ctx, args.into_run_args()) } else { @@ -114,8 +116,9 @@ pub(super) fn stop<'a>(ctx: &RunCtx<'a>, args: StopArgs) -> miette::Result( ctx: &RunCtx<'a>, - args: RestartArgs, + mut args: RestartArgs, ) -> miette::Result> { + args.if_present |= ctx.if_present; args.run(ctx.dir, (ctx.config)()?, matches!(ctx.reporter, ReporterType::Silent))?; Ok(Box::pin(std::future::ready(Ok(())))) } diff --git a/pnpm/crates/cli/src/cli_args/tests.rs b/pnpm/crates/cli/src/cli_args/tests.rs index 7cc0d052d3..bfe8cf2a3e 100644 --- a/pnpm/crates/cli/src/cli_args/tests.rs +++ b/pnpm/crates/cli/src/cli_args/tests.rs @@ -129,6 +129,61 @@ fn script_scoped_global_flags_parse_before_script_commands() { } } +#[test] +fn if_present_flag_parses_before_script_commands() { + for argv in [ + ["pacquet", "--if-present", "run", "build"].as_slice(), + ["pacquet", "--if-present", "test"].as_slice(), + ["pacquet", "--if-present", "start"].as_slice(), + ["pacquet", "--if-present", "stop"].as_slice(), + ["pacquet", "--if-present", "restart"].as_slice(), + ] { + let parsed = CliArgs::try_parse_from(argv).expect("parses top-level --if-present"); + assert!(parsed.if_present); + parsed.validate_command_scoped_global_options().expect("script command accepts flag"); + } +} + +/// The exact shape of the repo's own `test-pkgs-branch` script. +#[test] +fn if_present_flag_parses_before_fallback_command() { + let parsed = CliArgs::try_parse_from([ + "pacquet", + "--workspace-concurrency=1", + "--filter=...[origin/main]", + "--no-sort", + "--if-present", + ".test", + ]) + .expect("parses top-level --if-present with a fallback script"); + assert!(parsed.if_present); + assert!( + matches!(&parsed.command, CliCommand::External(command) if command.as_slice() == [".test"]), + ); + parsed.validate_command_scoped_global_options().expect("fallback command accepts flag"); +} + +#[test] +fn if_present_flag_rejects_non_script_commands() { + for argv in [ + ["pacquet", "--if-present", "install"].as_slice(), + ["pacquet", "--if-present", "publish"].as_slice(), + ["pacquet", "--if-present", "exec", "ls"].as_slice(), + ] { + let parsed = + CliArgs::try_parse_from(argv).expect("global parser accepts compatibility flag"); + let err = parsed + .validate_command_scoped_global_options() + .expect_err("non-script command rejects flag"); + assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument); + } + // Not `global = true` (the script subcommands declare their own + // `--if-present`), so after a non-script subcommand it fails at + // parse time instead of validation. + CliArgs::try_parse_from(["pacquet", "install", "--if-present"]) + .expect_err("install rejects --if-present at parse time"); +} + #[test] fn report_summary_global_flag_parses_for_publish() { for argv in [ diff --git a/pnpm/crates/cli/src/state.rs b/pnpm/crates/cli/src/state.rs index bf53d80c90..097d418356 100644 --- a/pnpm/crates/cli/src/state.rs +++ b/pnpm/crates/cli/src/state.rs @@ -76,7 +76,7 @@ impl State { }; Ok(State { config, - manifest: load_or_create_manifest(manifest_path)?, + manifest: load_or_create_manifest(manifest_path, config)?, lockfile, http_client: std::sync::Arc::new( ThrottledClient::for_installs( @@ -104,7 +104,16 @@ impl State { /// — pnpm reads every manifest base name. Alternate manifests stay /// read-only (see `pacquet_workspace::project_manifest`); commands /// that write the manifest back still require `package.json`. -fn load_or_create_manifest(manifest_path: PathBuf) -> Result { +/// +/// Inside a workspace, a missing root manifest is tolerated rather +/// than scaffolded: pnpm installs such a workspace with no root +/// importer and never creates a `package.json` there, so the stand-in +/// manifest stays in memory only. Commands that save the manifest +/// (`add`) still persist it through their explicit save. +fn load_or_create_manifest( + manifest_path: PathBuf, + config: &Config, +) -> Result { if !manifest_path.exists() { let project_dir = manifest_path.parent().expect("manifest path always has a parent dir"); if let Some((_, manifest)) = pacquet_workspace::try_read_project_manifest(project_dir) @@ -112,15 +121,8 @@ fn load_or_create_manifest(manifest_path: PathBuf) -> Result]` filter picking up a root-level change) - // would then run. Use the scaffold in memory without persisting it. - if project_dir.join("pnpm-workspace.yaml").exists() { - let value = PackageManifest::init_value_for(&manifest_path); - return Ok(PackageManifest::from_value(manifest_path, value)); + if config.workspace_dir.is_some() { + return Ok(PackageManifest::from_value(manifest_path, serde_json::json!({}))); } } manifest_path.pipe(PackageManifest::create_if_needed).map_err(InitStateError::Manifest) diff --git a/pnpm/crates/cli/tests/run.rs b/pnpm/crates/cli/tests/run.rs index 2d3368295c..db28c142d3 100644 --- a/pnpm/crates/cli/tests/run.rs +++ b/pnpm/crates/cli/tests/run.rs @@ -177,6 +177,26 @@ fn run_with_if_present_is_a_noop_for_missing_script() { drop(root); } +/// pnpm also accepts `--if-present` ahead of the script name +/// (`pnpm --if-present