pacquet/crates/workspace/src/root_finder/tests.rs (1)

121-121: ⚡ Quick win

Use an allocation-free lowercase key comparison in the fake env provider.

This currently allocates a String on each call. Compare against a borrowed lowercase key constant/literal instead.

♻️ Proposed fix

-            (name == WORKSPACE_DIR_ENV_VAR.to_lowercase())
+            (name == "npm_config_workspace_dir")
                 .then(|| OsString::from("/lowercase/root"))

As per coding guidelines, "Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pacquet/crates/workspace/src/root_finder/tests.rs` at line 121, The fake env
provider currently does an allocation by calling
WORKSPACE_DIR_ENV_VAR.to_lowercase() for each comparison (the expression name ==
WORKSPACE_DIR_ENV_VAR.to_lowercase()); change this to an allocation-free
comparison by either comparing against a pre-lowercased borrowed constant (e.g.
a &'static str like WORKSPACE_DIR_ENV_VAR_LOWER) or use
name.eq_ignore_ascii_case(WORKSPACE_DIR_ENV_VAR) /
name.eq_ignore_ascii_case(...) so no temporary String is created; update the
comparison in the fake env provider where WORKSPACE_DIR_ENV_VAR is referenced to
use one of these allocation-free approaches.

pacquet/crates/workspace/src/root_finder.rs (1)

124-124: ⚡ Quick win

Avoid per-call lowercase allocation for env-var fallback.

WORKSPACE_DIR_ENV_VAR.to_lowercase() allocates on every call. Prefer a lowercase &'static str constant and pass it directly.

♻️ Proposed fix

 pub(crate) const WORKSPACE_DIR_ENV_VAR: &str = "NPM_CONFIG_WORKSPACE_DIR";
+pub(crate) const WORKSPACE_DIR_ENV_VAR_LOWER: &str = "npm_config_workspace_dir";
@@
     Sys::var_os(WORKSPACE_DIR_ENV_VAR)
-        .or_else(|| Sys::var_os(&WORKSPACE_DIR_ENV_VAR.to_lowercase()))
+        .or_else(|| Sys::var_os(WORKSPACE_DIR_ENV_VAR_LOWER))
         .filter(|value| !value.is_empty())
         .map(PathBuf::from)

As per coding guidelines, "Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pacquet/crates/workspace/src/root_finder.rs` at line 124, The call uses
WORKSPACE_DIR_ENV_VAR.to_lowercase() which allocates on every invocation;
introduce a static &'static str constant (e.g. WORKSPACE_DIR_ENV_VAR_LOWERCASE)
holding the precomputed lowercase name and replace the to_lowercase() call in
the Sys::var_os fallback with that constant so no per-call allocation occurs;
update any references in root_finder.rs that currently call to_lowercase() on
WORKSPACE_DIR_ENV_VAR to use the new constant.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pacquet/crates/config/src/lib.rs (1)

1476-1494: 💤 Low value

HostNoEnvNoHome duplicates HostNoHome — prefer the module-level fake.

The HostNoEnvNoHome struct is functionally identical to HostNoHome (lines 966–986): both delegate EnvVar to Host, return None for EnvVarOs, and return None for GetHomeDir. The test can use HostNoHome directly.

Suggested simplification

     #[test]
     pub fn single_project_anchors_modules_at_cwd() {
-        struct HostNoEnvNoHome;
-        impl EnvVar for HostNoEnvNoHome {
-            fn var(name: &str) -> Option<String> {
-                Host::var(name)
-            }
-        }
-        impl EnvVarOs for HostNoEnvNoHome {
-            fn var_os(_: &str) -> Option<OsString> {
-                None
-            }
-        }
-        impl GetHomeDir for HostNoEnvNoHome {
-            fn home_dir() -> Option<PathBuf> {
-                None
-            }
-        }
         let tmp = tempdir().unwrap();
         let config =
-            Config::current::<HostNoEnvNoHome, _>(tmp.path(), Config::new).expect("config loads");
+            Config::current::<HostNoHome, _>(tmp.path(), Config::new).expect("config loads");
         assert_eq!(config.modules_dir, tmp.path().join("node_modules"));
         assert_eq!(config.virtual_store_dir, tmp.path().join("node_modules/.pnpm"));
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pacquet/crates/config/src/lib.rs` around lines 1476 - 1494, The new test
introduces a redundant fake HostNoEnvNoHome that duplicates the existing
module-level HostNoHome; replace HostNoEnvNoHome with the existing HostNoHome in
the call to Config::current (and remove the added struct impls), i.e. use
Config::current::<HostNoHome, _>(tmp.path(), Config::new) so the test reuses the
shared fake implementations (EnvVar, EnvVarOs, GetHomeDir) instead of
duplicating HostNoEnvNoHome.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes