mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-14 01:32:40 -04:00
fix(pacquet): address review findings on the verify-deps-before-run gate
- Keep the pre-run check read-only: pnpm's run path never writes pnpm-lock.yaml (only the install command restores it from the current lockfile), so the gate now uses a read-only stand-in check instead of regenerate_wanted_lockfile_if_missing, and a missing lockfile without a stand-in reports "Cannot find a lockfile" like pnpm instead of passing silently. - Match pnpm's env-var semantics for unrecognized values: pnpm assigns pnpm_config_verify_deps_before_run verbatim without validation, so a present-but-invalid value is truthy there (check runs, no action matches). Parse failures now map to VerifyDepsBeforeRun::True instead of being dropped, which also keeps a present env var overriding the CLI consistently. - Report "cannot check" straight from the missing workspace state, before the workspace-projects walk, sparing fresh projects the discovery cost on every run/exec. - Share the recursion-guard env-var name through a constant, cover the exec-path stamp with a test, and drop a test doc comment that only restated the assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -178,10 +178,8 @@ pub(super) fn spawn_in_dir(
|
||||
// when no `userAgent` is configured (makeEnv.ts:30). pacquet has
|
||||
// no `userAgent` setting yet, so it always takes that default.
|
||||
cmd.env("npm_config_user_agent", "pnpm");
|
||||
// A nested `pnpm run` / `pnpm exec` inside the command must skip
|
||||
// the verify-deps-before-run check (same stamp as the lifecycle
|
||||
// env builder; the env var outranks every other config source).
|
||||
cmd.env("pnpm_config_verify_deps_before_run", "false");
|
||||
// Same recursion-guard stamp as the lifecycle env builder.
|
||||
cmd.env(pacquet_executor::VERIFY_DEPS_BEFORE_RUN_ENV, "false");
|
||||
if let Some(name) = read_package_name(dir) {
|
||||
cmd.env("PNPM_PACKAGE_NAME", name);
|
||||
}
|
||||
|
||||
@@ -182,7 +182,7 @@ impl ConfigOverrides {
|
||||
}
|
||||
|
||||
fn verify_deps_env_is_set() -> bool {
|
||||
["PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN", "pnpm_config_verify_deps_before_run"]
|
||||
["PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN", pacquet_executor::VERIFY_DEPS_BEFORE_RUN_ENV]
|
||||
.iter()
|
||||
.any(|name| std::env::var(name).is_ok_and(|value| !value.is_empty()))
|
||||
}
|
||||
|
||||
@@ -42,10 +42,6 @@ fn default_install_action_installs_before_running_the_script() {
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// `error` fails a fresh project ("Cannot check whether dependencies
|
||||
/// are outdated"), passes after an install, still passes after an
|
||||
/// mtime-only manifest rewrite, and fails again once the manifest no
|
||||
/// longer matches the lockfile.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn error_action_follows_the_dependency_state() {
|
||||
@@ -84,6 +80,27 @@ fn error_action_follows_the_dependency_state() {
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
// Deleting pnpm-lock.yaml in a dependency-less project leaves no
|
||||
// current lockfile to stand in for it, so the check fails like
|
||||
// pnpm's RUN_CHECK_DEPS_LOCKFILE_NOT_FOUND — and the pre-run check
|
||||
// must not recreate the file (pnpm's run path never restores the
|
||||
// lockfile; only the install command does).
|
||||
fs::remove_file(workspace.join("pnpm-lock.yaml")).expect("remove pnpm-lock.yaml");
|
||||
let output = pacquet_in(&workspace)
|
||||
.with_args(["--config.verify-deps-before-run=error", "run", "hello"])
|
||||
.output()
|
||||
.expect("spawn pacquet run");
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
eprintln!("STDERR:\n{stderr}\n");
|
||||
assert!(!output.status.success(), "a missing lockfile must fail");
|
||||
assert!(stderr.contains("Cannot find a lockfile in"), "expected the lockfile error:\n{stderr}");
|
||||
assert!(
|
||||
!workspace.join("pnpm-lock.yaml").exists(),
|
||||
"the pre-run check must not write pnpm-lock.yaml",
|
||||
);
|
||||
pacquet_in(&workspace).with_arg("install").assert().success();
|
||||
assert!(workspace.join("pnpm-lock.yaml").exists(), "install must restore the lockfile");
|
||||
|
||||
// A manifest that no longer matches the lockfile must fail again.
|
||||
let mut manifest: serde_json::Value = serde_json::from_str(
|
||||
&fs::read_to_string(workspace.join("package.json")).expect("read package.json"),
|
||||
@@ -213,6 +230,49 @@ fn env_var_outranks_the_cli_config_override() {
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// The exec path stamps the same recursion guard as the lifecycle env
|
||||
/// builder.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn exec_children_get_the_check_disabled_through_their_env() {
|
||||
let CommandTempCwd { pacquet, root, workspace, .. } = CommandTempCwd::init();
|
||||
write_manifest(&workspace, &workspace.join("marker.txt"));
|
||||
|
||||
pacquet
|
||||
.with_args([
|
||||
"--config.verify-deps-before-run=false",
|
||||
"exec",
|
||||
"sh",
|
||||
"-c",
|
||||
r#"[ "$pnpm_config_verify_deps_before_run" = "false" ]"#,
|
||||
])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// pnpm assigns the `pnpm_config_verify_deps_before_run` env var
|
||||
/// verbatim, so an unrecognized value is truthy there: the check runs
|
||||
/// but matches no action, and the script proceeds.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn unrecognized_env_value_checks_without_acting() {
|
||||
let CommandTempCwd { pacquet, root, workspace, .. } = CommandTempCwd::init();
|
||||
let marker = workspace.join("marker.txt");
|
||||
write_manifest(&workspace, &marker);
|
||||
|
||||
pacquet
|
||||
.with_env("pnpm_config_verify_deps_before_run", "definitely-not-an-action")
|
||||
.with_args(["--config.verify-deps-before-run=error", "run", "hello"])
|
||||
.assert()
|
||||
.success();
|
||||
assert!(marker.exists(), "the script must run");
|
||||
assert!(!workspace.join("node_modules").exists(), "no action may fire");
|
||||
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// `pnpm exec` runs the same gate as `pnpm run`.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
|
||||
@@ -202,7 +202,17 @@ impl WorkspaceSettings {
|
||||
json_field!(trust_lockfile, "TRUST_LOCKFILE");
|
||||
enum_field!(trust_policy, "TRUST_POLICY", TrustPolicy);
|
||||
enum_field!(pm_on_fail, "PM_ON_FAIL", PmOnFail);
|
||||
enum_field!(verify_deps_before_run, "VERIFY_DEPS_BEFORE_RUN", VerifyDepsBeforeRun);
|
||||
// pnpm assigns this env var verbatim without validation, so an
|
||||
// unrecognized value is truthy there: the check runs but matches
|
||||
// no action. `True` reproduces that, and keeps a present-but-
|
||||
// invalid value overriding the other config layers the way a
|
||||
// valid one does.
|
||||
if let Some(s) = read_env::<Sys>("VERIFY_DEPS_BEFORE_RUN") {
|
||||
settings.verify_deps_before_run = Some(
|
||||
parse_json_or_string::<VerifyDepsBeforeRun>(&s)
|
||||
.unwrap_or(VerifyDepsBeforeRun::True),
|
||||
);
|
||||
}
|
||||
enum_field!(audit_level, "AUDIT_LEVEL", AuditLevel);
|
||||
json_field!(audit_config, "AUDIT_CONFIG");
|
||||
json_field!(trust_policy_exclude, "TRUST_POLICY_EXCLUDE");
|
||||
|
||||
@@ -9,7 +9,7 @@ pub use lifecycle::{
|
||||
LifecycleScriptError, PROJECT_LIFECYCLE_STAGES, RunPostinstallHooks, push_script_arg,
|
||||
run_lifecycle_hook, run_postinstall_hooks, run_project_lifecycle_scripts,
|
||||
};
|
||||
pub use make_env::{EnvBuild, EnvOptions, build_env};
|
||||
pub use make_env::{EnvBuild, EnvOptions, VERIFY_DEPS_BEFORE_RUN_ENV, build_env};
|
||||
pub use run_script::{RunScript, RunScriptError, run_script};
|
||||
pub use shell::{ScriptShellError, SelectedShell, select_shell};
|
||||
|
||||
|
||||
@@ -5,6 +5,12 @@ use std::{
|
||||
path::{Path, PathBuf},
|
||||
};
|
||||
|
||||
/// Env var pnpm stamps as `false` into every spawned script and exec
|
||||
/// child so a nested `pnpm run` / `pnpm exec` skips the
|
||||
/// verify-deps-before-run check; the config layer reads it back with
|
||||
/// priority over every other source of that setting (pnpm/pnpm#10060).
|
||||
pub const VERIFY_DEPS_BEFORE_RUN_ENV: &str = "pnpm_config_verify_deps_before_run";
|
||||
|
||||
/// Inputs needed to build the env for a single lifecycle hook spawn:
|
||||
/// the package context, the per-call stamps the hook runner adds, and
|
||||
/// the caller-supplied `extra_env` overrides.
|
||||
@@ -94,12 +100,9 @@ pub fn build_env(
|
||||
env.insert("npm_config_user_agent".into(), ua.to_string());
|
||||
}
|
||||
|
||||
// A nested `pnpm run` / `pnpm exec` inside a script must skip the
|
||||
// verify-deps-before-run check — this env var outranks every other
|
||||
// source of that setting on the config side, breaking the recursion
|
||||
// a spawned install's lifecycle scripts would otherwise enter
|
||||
// (pnpm stamps the same value via `config.extraEnv`).
|
||||
env.insert("pnpm_config_verify_deps_before_run".into(), "false".into());
|
||||
// Breaks the recursion a spawned install's lifecycle scripts would
|
||||
// otherwise enter (pnpm stamps the same value via `config.extraEnv`).
|
||||
env.insert(VERIFY_DEPS_BEFORE_RUN_ENV.into(), "false".into());
|
||||
|
||||
// 4. `extra_env` is applied last among the base env writes, so it
|
||||
// overrides anything stamped above.
|
||||
|
||||
@@ -2394,6 +2394,13 @@ pub fn check_deps_status_before_run_at(
|
||||
},
|
||||
None => None,
|
||||
};
|
||||
// pnpm reports "cannot check" straight from the missing workspace
|
||||
// state, before any project discovery — a fresh project (the common
|
||||
// out-of-sync case) must not pay for the workspace-projects walk
|
||||
// only to reach the same verdict inside the check.
|
||||
if !matches!(pacquet_workspace_state::load_workspace_state(&workspace_root), Ok(Some(_))) {
|
||||
return cannot_check();
|
||||
}
|
||||
let catalogs = match config.catalogs.clone() {
|
||||
Some(catalogs) => catalogs,
|
||||
None => match get_catalogs_from_workspace_manifest(workspace_manifest.as_ref()) {
|
||||
|
||||
@@ -1469,18 +1469,18 @@ pub fn check_deps_status_before_run(check: &OptimisticRepeatInstallCheck<'_>) ->
|
||||
}
|
||||
|
||||
if modified.is_empty() && !lockfile_modified {
|
||||
return match regenerate_wanted_lockfile_if_missing(check, None) {
|
||||
return match missing_wanted_lockfile_stand_in_ok(check) {
|
||||
Ok(()) => RunDepsStatus::UpToDate,
|
||||
Err(reason) => outdated(reason.to_string()),
|
||||
Err(reason) => outdated(reason),
|
||||
};
|
||||
}
|
||||
|
||||
let projects_to_check: Vec<&ManifestStat<'_>> =
|
||||
if lockfile_modified { manifest_stats.iter().collect() } else { modified };
|
||||
match modified_manifests_match_lockfile(check, &state, &projects_to_check) {
|
||||
Ok(loaded_current) => {
|
||||
if let Err(reason) = regenerate_wanted_lockfile_if_missing(check, loaded_current) {
|
||||
return outdated(reason.to_string());
|
||||
Ok(_) => {
|
||||
if let Err(reason) = missing_wanted_lockfile_stand_in_ok(check) {
|
||||
return outdated(reason);
|
||||
}
|
||||
if is_workspace_install {
|
||||
let mut new_state = crate::install::build_workspace_state(
|
||||
@@ -1512,6 +1512,25 @@ pub fn check_deps_status_before_run(check: &OptimisticRepeatInstallCheck<'_>) ->
|
||||
}
|
||||
}
|
||||
|
||||
/// Read-only twin of [`regenerate_wanted_lockfile_if_missing`] for the
|
||||
/// run gate: pnpm's run-path check never writes `pnpm-lock.yaml` (only
|
||||
/// the install command restores it from the current lockfile), so a
|
||||
/// missing wanted lockfile passes exactly when the current lockfile can
|
||||
/// stand in for it, and the check leaves the workspace untouched.
|
||||
fn missing_wanted_lockfile_stand_in_ok(
|
||||
check: &OptimisticRepeatInstallCheck<'_>,
|
||||
) -> Result<(), String> {
|
||||
if check.lockfile.is_loaded_or_on_disk() || !check.config.lockfile {
|
||||
return Ok(());
|
||||
}
|
||||
match Lockfile::load_current_from_virtual_store_dir(&check.config.virtual_store_dir) {
|
||||
Ok(Some(_)) => Ok(()),
|
||||
Ok(None) | Err(_) => {
|
||||
Err(format!("Cannot find a lockfile in {}", check.workspace_root.display()))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// The `pnpm install` arguments reproducing the dependency groups the
|
||||
/// workspace state recorded, so the `install` / `prompt` actions rerun
|
||||
/// the same kind of install the project last had (pnpm's
|
||||
|
||||
Reference in New Issue
Block a user