feat: default fast execution for run and exec#2
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved the Benchmark GitHub Actions workflow and one README sentence; made native-mode the default and added Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Resolve as Resolver
participant Project as ProjectState
participant NodeProbe as NodeProbe
participant PM as PackageManager
CLI->>Resolve: resolve_node_run(args, ctx)
Resolve->>Project: ctx.project_state() (lazy scan)
Resolve->>Resolve: attempt_nr(pm_hint, args, ctx)
alt native_mode & native eligible
Resolve-->>CLI: Resolved Native execution (mode: native)
else native_mode & native ineligible
Resolve->>NodeProbe: build_node_run_exec_if_safe(ctx, script)
alt node --run supported & safe
NodeProbe-->>Resolve: Node --run execution (mode: node-run)
Resolve-->>CLI: Resolved NodeRun execution
else unsafe/unsupported
Resolve->>PM: build_exec(Intent::Run) (mode: package-manager)
PM-->>Resolve: Package-manager execution
Resolve-->>CLI: Resolved PackageManager execution
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/cli.rs (1)
549-558:⚠️ Potential issue | 🟡 MinorMention
--fastin the conflict error.
--fast --no-nativecurrently reports--native conflicts with --no-native, which points at a flag the user never passed.✏️ Suggested tweak
- Err(HniError::parse("--native conflicts with --no-native")) + Err(HniError::parse("--native/--fast conflicts with --no-native"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/cli.rs` around lines 549 - 558, The conflict error message in set_native_override reports "--native conflicts with --no-native" which is misleading when the true source is the --fast flag; update the error text in set_native_override (the HniError::parse call) to mention --fast as well (for example "--native/--fast conflicts with --no-native" or otherwise include "--fast") so users who passed --fast see the correct conflict candidate; change only the string passed to HniError::parse in the match arm that handles Some(existing) if existing != value.
🧹 Nitpick comments (2)
tests/resolve_matrix.rs (1)
327-330: Use a drop guard forHNI_REAL_NODEcleanup.These cases restore the process env manually after the
unwrap()/asserts. If one assertion fails early, later tests in the same process can inherit the fake node path and fail for the wrong reason. A smallDrop-based helper would make this suite non-leaky.Also applies to: 355-358, 384-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/resolve_matrix.rs` around lines 327 - 330, The tests set HNI_REAL_NODE using support::set_var and then call resolve::resolve_node_run, but they manually call support::remove_var afterwards which leaks if an assertion panics; create a small Drop-based helper (e.g., EnvVarGuard or HniRealNodeGuard) that captures the previous value (or absence) of the HNI_REAL_NODE env var in its constructor when you call support::set_var and restores or removes it in Drop, then replace the set_var/remove_var pairs in the failing test blocks (and the similar locations around resolve_node_run calls at the other lines mentioned) with creation of this guard so the environment is always restored even if tests panic or unwind.src/core/resolve/detect.rs (1)
24-69: Make the validated path harder to skip.
detect_for_action()now returns a package manager even when the binary is unavailable, and every external-exec caller has to remember to addensure_detected_available()later. I'd consider renaming the raw helper or returning a validated wrapper so future resolver paths don't regress to latecommand not foundfailures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/resolve/detect.rs` around lines 24 - 69, detect_for_action currently returns an AgentResolution even if the agent binary is missing, relying on callers to call ensure_detected_available; change this so callers can't easily forget the validation: rename the existing detect_for_action to detect_raw_for_action (or similar) to preserve behavior, then implement a new detect_for_action that calls the raw detector and immediately validates via ensure_package_manager_available (using resolution.pm and resolution.version_hint.as_deref()) and maps validation errors into HniError::detection before returning the validated AgentResolution; update call sites to use the new detect_for_action or the raw name where unvalidated behavior is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/native.rs`:
- Around line 157-158: The chained bin resolution expression assigned to
bin_path is misformatted for rustfmt; update the wrapping so rustfmt accepts it
by placing the first call on its own line and the or_else chain indented on the
next line (e.g., start with let bin_path = resolve_local_bin(bin_name,
&bin_paths) and then call .or_else(|) on the next line pointing to
state.resolve_declared_package_bin(bin_name)); ensure you reference
resolve_local_bin, resolve_declared_package_bin and the bin_path assignment and
keep commas/parentheses unchanged so the expression remains a single let
binding.
In `@src/core/resolve/build.rs`:
- Line 97: The eager call to detect_for_action(ctx, false) aborts fast-path
runs; change the flow so build_node_run_exec_if_safe(...) is attempted first and
only call detect_for_action(...) when the safe node-run path returns None and
you need the package-manager fallback. Concretely, remove or defer the immediate
assignment to detected from detect_for_action at the earlier sites and instead:
call build_node_run_exec_if_safe(ctx, ...) first, if it returns Some(exec) use
that, otherwise call detect_for_action(ctx, false) and proceed with the existing
package-manager fallback logic using the detected value; apply the same change
at both occurrences where detect_for_action is currently invoked.
- Around line 117-126: The code returns build_legacy_node_run_exec whenever
ctx.config.run_agent == RunAgent::Node even if build_node_run_exec_if_safe
returned None (meaning unsupported/unsafe/missing script), which bypasses the
new safety gate; change the logic so that build_legacy_node_run_exec is only
returned when build_node_run_exec_if_safe actually produced a Some(resolved)
(i.e., keep the current return inside the Some branch) and remove/avoid the
unconditional fallback to build_legacy_node_run_exec when
build_node_run_exec_if_safe returned None; update both call sites that use
build_node_run_exec_if_safe and the conditional on ctx.config.run_agent ==
RunAgent::Node (the occurrences around build_node_run_exec_if_safe and
build_legacy_node_run_exec) so a None result falls back to the package manager
path instead of calling build_legacy_node_run_exec.
In `@src/core/resolve/context.rs`:
- Around line 15-45: ResolveContext currently exposes a mutable cwd while
caching ProjectState in project_state, causing stale state if cwd is changed
after the first project_state() or detect() call; fix by making cwd
private/immutable (remove pub on cwd and provide read-only accessor) so callers
cannot mutate it after construction, or—if public mutation is required—add cache
invalidation logic: whenever cwd is changed (e.g., provide a set_cwd method or
wrap existing mutation points), call project_state.take() or recreate/reset
project_state so ProjectState::scan will run against the new cwd; update uses of
ResolveContext::new, ResolveContext::project_state, and ResolveContext::detect
accordingly.
In `@src/core/types.rs`:
- Around line 43-49: The ExecutionMode enum and its external() constructor are
mislabeling non-package-manager executions; add a neutral variant (e.g.,
External or Passthrough) to ExecutionMode (in the enum where
PackageManager/NodeRun/Native/Internal are defined) and change external() to
return that neutral variant instead of PackageManager; then update callers that
need specific semantics (notably resolve_node_passthrough() in resolve/build.rs)
to either construct the correct explicit mode (e.g., ExecutionMode::NodeRun) or
continue using the new neutral ExecutionMode::External/Passthrough where
appropriate; also apply the same change to the other external() usage locations
referenced around lines 88-95 so nothing is implicitly labeled PackageManager.
In `@src/platform/node.rs`:
- Around line 75-82: The current probe_node_run_support function runs `node
--help` without a timeout and can hang; update probe_node_run_support to spawn
the child (using Command::new(node_path).arg("--help").spawn()), wait with a
short Duration (e.g. a few hundred ms) and, on timeout, kill/terminate the child
and return false (treat timeout as unsupported). After successful completion,
capture stdout/stderr and pass them to help_text_supports_run as before; ensure
any spawn/wait/kill errors are handled by returning false. Use the existing
function name probe_node_run_support and helper help_text_supports_run when
locating where to implement the change.
---
Outside diff comments:
In `@src/app/cli.rs`:
- Around line 549-558: The conflict error message in set_native_override reports
"--native conflicts with --no-native" which is misleading when the true source
is the --fast flag; update the error text in set_native_override (the
HniError::parse call) to mention --fast as well (for example "--native/--fast
conflicts with --no-native" or otherwise include "--fast") so users who passed
--fast see the correct conflict candidate; change only the string passed to
HniError::parse in the match arm that handles Some(existing) if existing !=
value.
---
Nitpick comments:
In `@src/core/resolve/detect.rs`:
- Around line 24-69: detect_for_action currently returns an AgentResolution even
if the agent binary is missing, relying on callers to call
ensure_detected_available; change this so callers can't easily forget the
validation: rename the existing detect_for_action to detect_raw_for_action (or
similar) to preserve behavior, then implement a new detect_for_action that calls
the raw detector and immediately validates via ensure_package_manager_available
(using resolution.pm and resolution.version_hint.as_deref()) and maps validation
errors into HniError::detection before returning the validated AgentResolution;
update call sites to use the new detect_for_action or the raw name where
unvalidated behavior is intentional.
In `@tests/resolve_matrix.rs`:
- Around line 327-330: The tests set HNI_REAL_NODE using support::set_var and
then call resolve::resolve_node_run, but they manually call support::remove_var
afterwards which leaks if an assertion panics; create a small Drop-based helper
(e.g., EnvVarGuard or HniRealNodeGuard) that captures the previous value (or
absence) of the HNI_REAL_NODE env var in its constructor when you call
support::set_var and restores or removes it in Drop, then replace the
set_var/remove_var pairs in the failing test blocks (and the similar locations
around resolve_node_run calls at the other lines mentioned) with creation of
this guard so the environment is always restored even if tests panic or unwind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b17ac87d-0912-46c4-a075-ce5b930d8f50
📒 Files selected for processing (21)
.github/workflows/benchmark.ymlREADME.mdbenchmark/profile.shsrc/app/cli.rssrc/app/dispatch.rssrc/app/doctor.rssrc/app/help.rssrc/core/config.rssrc/core/detect.rssrc/core/native.rssrc/core/resolve/build.rssrc/core/resolve/context.rssrc/core/resolve/detect.rssrc/core/resolve/mod.rssrc/core/types.rssrc/platform/node.rstests/cli_contract.rstests/config_detect.rstests/init_contract.rstests/native_execution.rstests/resolve_matrix.rs
💤 Files with no reviewable changes (2)
- README.md
- .github/workflows/benchmark.yml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/core/resolve/build.rs (2)
107-108:⚠️ Potential issue | 🟠 MajorDefer agent detection until the run fallback is actually needed.
Line 108 and Line 187 still force
detect_for_action()before the native /node --runbranches run. That meansnr/node runcan still fail in a repo with a runnablepackage.jsonscript but no detectable package manager, even though the fast path itself does not need the delegated fallback yet. Move the hard detection step after the fast-path attempts.Also applies to: 187-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/resolve/build.rs` around lines 107 - 108, The resolve_nr function is calling detect_for_action() too early and forces agent detection before the fast-path native / `node --run` branches execute; update resolve_nr to postpone invoking detect_for_action() (and any use of its result) until after you attempt the fast-path resolution (the native branch and node --run branch) so that a runnable package.json script succeeds without a detectable package manager, and similarly move the deferred detection in the fallback logic that currently references detect_for_action() (also mentioned at the later fallback location) so detection only occurs when the delegated fallback is actually needed.
136-137:⚠️ Potential issue | 🟠 MajorDon't route back to raw
node --runafter the safety check already rejected it.Once
build_node_run_exec_if_safe()returnsNone, Lines 136-137 and 161-163 still emitbuild_legacy_node_run_exec(). That bypasses the guard for missing scripts, unsupported env expansion, pre/post hooks, or unsupported Node builds and lands on the exactnode --runpath that was just ruled out. This branch should fall back to the package-manager path instead.Also applies to: 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/resolve/build.rs` around lines 136 - 137, The code currently returns build_legacy_node_run_exec whenever ctx.config.run_agent == RunAgent::Node even if build_node_run_exec_if_safe() previously returned None; change both places (the branch at the first occurrence and the later one) to first call build_node_run_exec_if_safe(args, ctx) and, if it returns Some(exec), return that, but if it returns None do NOT call build_legacy_node_run_exec — instead fall back to the package-manager execution path used elsewhere in this function (i.e., follow the same fallback logic as the non-Node branches). Ensure you replace the unconditional build_legacy_node_run_exec calls with this guarded behavior around build_node_run_exec_if_safe().src/core/resolve/context.rs (1)
16-20:⚠️ Potential issue | 🟠 MajorCached project state can go stale after
cwdmutation.Line 17 keeps
cwdpublic while Lines 19-20 cache the firstProjectState. Any laterctx.cwdupdate leaves detection,.binlookup, and Yarn PnP checks pinned to the old tree. Please makecwdimmutable/private or invalidateproject_stateon every mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/resolve/context.rs` around lines 16 - 20, ResolveContext currently exposes pub cwd while caching a single ProjectState in project_state (OnceLock), which lets external mutations to cwd leave project_state stale; fix by making cwd private (change pub cwd: PathBuf to cwd: PathBuf) and expose a read-only accessor like fn cwd(&self) -> &Path to prevent external mutation, or if you must allow cwd changes replace project_state/OnceLock with a resettable container and add a set_cwd(&mut self, new: PathBuf) that updates cwd and clears/invalidates project_state before returning (referencing ResolveContext, cwd, project_state, ProjectState, OnceLock).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/cli.rs`:
- Around line 357-361: The invocation argument parser currently omits "np" and
"ns", so update the PossibleValuesParser list passed to Arg::new("invocation")
to include "np" and "ns" and also ensure internal_invocation() recognizes these
strings and maps them to InvocationKind::Np and InvocationKind::Ns (or better:
factor the mapping into a shared lookup used by both the Arg parser and
internal_invocation()/dispatch_invocation() to avoid drift); touch the other
parser block around the 415-424 region the comment mentions so both places use
the same canonical set or mapping for invocation names.
In `@src/app/dispatch.rs`:
- Around line 55-61: Change the matches! that computes
verify_package_manager_availability to borrow parsed.command instead of moving
it: use matches!(&parsed.command, ParsedCommand::Execute { .. }) so
parsed.command remains usable later; keep the same logic and pass the resulting
boolean into ResolveContext::with_package_manager_checks (which takes
parsed.cwd.clone(), config.clone(), verify_package_manager_availability).
In `@src/core/resolve/build.rs`:
- Around line 253-256: In resolve_nlx, don't call detect_for_action() before
attempting the native local-bin fast path; instead, if ctx.config.native_mode is
true, first call native::attempt_nlx(detected_pm?, &args, ctx) but without
earlier detection—invoke native::attempt_nlx directly and only call
detect_for_action() after that attempt fails so you can supply the detected
package manager for the delegated fallback; update resolve_nlx to defer
detect_for_action() and only use its result for the fallback branch.
---
Duplicate comments:
In `@src/core/resolve/build.rs`:
- Around line 107-108: The resolve_nr function is calling detect_for_action()
too early and forces agent detection before the fast-path native / `node --run`
branches execute; update resolve_nr to postpone invoking detect_for_action()
(and any use of its result) until after you attempt the fast-path resolution
(the native branch and node --run branch) so that a runnable package.json script
succeeds without a detectable package manager, and similarly move the deferred
detection in the fallback logic that currently references detect_for_action()
(also mentioned at the later fallback location) so detection only occurs when
the delegated fallback is actually needed.
- Around line 136-137: The code currently returns build_legacy_node_run_exec
whenever ctx.config.run_agent == RunAgent::Node even if
build_node_run_exec_if_safe() previously returned None; change both places (the
branch at the first occurrence and the later one) to first call
build_node_run_exec_if_safe(args, ctx) and, if it returns Some(exec), return
that, but if it returns None do NOT call build_legacy_node_run_exec — instead
fall back to the package-manager execution path used elsewhere in this function
(i.e., follow the same fallback logic as the non-Node branches). Ensure you
replace the unconditional build_legacy_node_run_exec calls with this guarded
behavior around build_node_run_exec_if_safe().
In `@src/core/resolve/context.rs`:
- Around line 16-20: ResolveContext currently exposes pub cwd while caching a
single ProjectState in project_state (OnceLock), which lets external mutations
to cwd leave project_state stale; fix by making cwd private (change pub cwd:
PathBuf to cwd: PathBuf) and expose a read-only accessor like fn cwd(&self) ->
&Path to prevent external mutation, or if you must allow cwd changes replace
project_state/OnceLock with a resettable container and add a set_cwd(&mut self,
new: PathBuf) that updates cwd and clears/invalidates project_state before
returning (referencing ResolveContext, cwd, project_state, ProjectState,
OnceLock).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18db1b36-1852-47d6-9b42-02135416f7d4
📒 Files selected for processing (11)
src/app/cli.rssrc/app/dispatch.rssrc/core/native.rssrc/core/resolve/build.rssrc/core/resolve/context.rssrc/core/resolve/detect.rssrc/core/types.rssrc/platform/node.rstests/cli_contract.rstests/native_execution.rstests/resolve_matrix.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/types.rs
- src/platform/node.rs
- tests/cli_contract.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/platform/node.rs (1)
315-374: Use one shared env-test guard for these cases.These tests only serialize against each other. They do not coordinate with the separate environment lock already used by the integration suite, so
HNI_REAL_NODEcan still race under default parallel test execution. Please move them behind the same process-wide test helper (or into the integration suite) instead of keeping a second lock here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/node.rs` around lines 315 - 374, These two tests (real_node_supports_run_uses_env_override and real_node_supports_run_times_out_for_hanging_help) currently use the local ENV_LOCK guard and therefore only serialize against each other; replace that local lock usage with the project’s single process-wide env-test helper so they serialize with the integration suite as well. Specifically, remove the ENV_LOCK.get_or_init()...lock() block at the start of each test and instead wrap the test body that touches REAL_NODE_ENV in the shared helper (the existing process-wide env-test helper used elsewhere in the repo), preserving the same logic that sets/restores REAL_NODE_ENV and filesystem setup so behavior of real_node_supports_run remains unchanged.tests/resolve_matrix.rs (1)
389-392: Consider RAII guard forHNI_REAL_NODEcleanup.The current pattern sets
HNI_REAL_NODEat line 389 and removes it at line 392, but if the test panics between these lines, the env var won't be cleaned up. This could affect other tests. Consider using an RAII guard similar towith_path_overridefor consistency and safety.♻️ Suggested approach
fn with_real_node_override<T>(path: &Path, f: impl FnOnce() -> T) -> T { struct RealNodeGuard(Option<std::ffi::OsString>); impl Drop for RealNodeGuard { fn drop(&mut self) { match &self.0 { Some(value) => support::set_var("HNI_REAL_NODE", value), None => support::remove_var("HNI_REAL_NODE"), } } } let original = std::env::var_os("HNI_REAL_NODE"); support::set_var("HNI_REAL_NODE", path); let guard = RealNodeGuard(original); let out = f(); drop(guard); out }Then usage becomes:
with_real_node_override(&fake_node, || { let ctx = ResolveContext::new(...); let resolved = resolve::resolve_node_run(...); // assertions });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/resolve_matrix.rs` around lines 389 - 392, Replace the manual set/remove of the HNI_REAL_NODE env var with an RAII guard to ensure cleanup on panic: implement a helper like with_real_node_override that captures the original std::env::var_os("HNI_REAL_NODE"), calls support::set_var("HNI_REAL_NODE", &fake_node), and returns a guard whose Drop restores the original value or calls support::remove_var; then wrap the ResolveContext::new(...) / resolve::resolve_node_run(...) invocation inside that helper (similar to with_path_override) so the env var is always restored even if the test panics.src/app/dispatch.rs (1)
157-157: Clarify the detection fallback pattern.The
.or_else(|_| detect(ctx.cwd(), &ctx.config))suggestsctx.detect()might fail inexplainmode while standalonedetectmight succeed. However, both should have the same behavior sincectx.detect()callsdetect()internally. This fallback appears to be defensive code that may never be exercised.Consider either documenting why this fallback is needed or simplifying to just
ctx.detect()if the fallback is not necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/dispatch.rs` at line 157, The fallback using ctx.detect().or_else(|_| detect(ctx.cwd(), &ctx.config)) is redundant because ctx.detect() already delegates to detect(), so either remove the fallback and call ctx.detect() directly, or if you intend to keep defensive behavior, add a brief comment above the call explaining why the standalone detect(ctx.cwd(), &ctx.config) is necessary (for example: explain mode differences or historical edge cases). Update the code around ctx.detect() in dispatch.rs accordingly and ensure tests still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/resolve/build.rs`:
- Around line 499-532: The calls to ctx.project_state() currently use ? which
propagates errors instead of falling back; fetch project_state once and convert
failures into Ok(None) (e.g., use a match or if let to return Ok(None) on Err),
then reuse that project_state for nearest_package() and has_yarn_pnp_loader()
checks; update build_node_run_exec_if_safe to obtain project_state with
error-to-Ok(None) handling and then proceed with let Some(pkg) =
project_state.nearest_package() else return Ok(None) and use
project_state.has_yarn_pnp_loader() where needed.
In `@src/platform/node.rs`:
- Around line 90-113: The current probe inspects stdout/stderr even when the
child process exits with a non-zero status, which can falsely detect support;
modify the logic around the child process handling in the node --help probe (the
code that uses child.try_wait(), child.wait(), and help_text_supports_run) to
check the exit status (Status from child.wait()) and if !status.success() return
false (treat as unsupported) before calling help_text_supports_run on
stdout/stderr; ensure you still drain stdout/stderr safely after waiting, but do
not consider their contents unless status.success() is true.
---
Nitpick comments:
In `@src/app/dispatch.rs`:
- Line 157: The fallback using ctx.detect().or_else(|_| detect(ctx.cwd(),
&ctx.config)) is redundant because ctx.detect() already delegates to detect(),
so either remove the fallback and call ctx.detect() directly, or if you intend
to keep defensive behavior, add a brief comment above the call explaining why
the standalone detect(ctx.cwd(), &ctx.config) is necessary (for example: explain
mode differences or historical edge cases). Update the code around ctx.detect()
in dispatch.rs accordingly and ensure tests still pass.
In `@src/platform/node.rs`:
- Around line 315-374: These two tests (real_node_supports_run_uses_env_override
and real_node_supports_run_times_out_for_hanging_help) currently use the local
ENV_LOCK guard and therefore only serialize against each other; replace that
local lock usage with the project’s single process-wide env-test helper so they
serialize with the integration suite as well. Specifically, remove the
ENV_LOCK.get_or_init()...lock() block at the start of each test and instead wrap
the test body that touches REAL_NODE_ENV in the shared helper (the existing
process-wide env-test helper used elsewhere in the repo), preserving the same
logic that sets/restores REAL_NODE_ENV and filesystem setup so behavior of
real_node_supports_run remains unchanged.
In `@tests/resolve_matrix.rs`:
- Around line 389-392: Replace the manual set/remove of the HNI_REAL_NODE env
var with an RAII guard to ensure cleanup on panic: implement a helper like
with_real_node_override that captures the original
std::env::var_os("HNI_REAL_NODE"), calls support::set_var("HNI_REAL_NODE",
&fake_node), and returns a guard whose Drop restores the original value or calls
support::remove_var; then wrap the ResolveContext::new(...) /
resolve::resolve_node_run(...) invocation inside that helper (similar to
with_path_override) so the env var is always restored even if the test panics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64fc72e1-cea4-4cd1-b6c5-f4717ba2167d
📒 Files selected for processing (18)
src/app/cli.rssrc/app/dispatch.rssrc/app/doctor.rssrc/app/version.rssrc/core/batch.rssrc/core/native.rssrc/core/resolve/build.rssrc/core/resolve/context.rssrc/core/resolve/detect.rssrc/core/types.rssrc/features/node_shim.rssrc/features/np.rssrc/features/nr.rssrc/features/ns.rssrc/features/nun.rssrc/platform/node.rstests/cli_contract.rstests/resolve_matrix.rs
✅ Files skipped from review due to trivial changes (1)
- src/core/resolve/context.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cli_contract.rs
Comment on lines
+499
to
+532
| fn build_node_run_exec_if_safe( | ||
| pm: Option<PackageManager>, | ||
| args: &[String], | ||
| ctx: &ResolveContext, | ||
| has_if_present: bool, | ||
| ) -> HniResult<Option<ResolvedExecution>> { | ||
| if pm == Some(PackageManager::Deno) { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| if !real_node_supports_run() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| if has_if_present { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| let Some(pkg) = ctx.project_state()?.nearest_package() else { | ||
| return Ok(None); | ||
| }; | ||
| let scripts = pkg.manifest.scripts.unwrap_or_default(); | ||
| let script_name = args.first().cloned().unwrap_or_else(|| "start".to_string()); | ||
| let Some(script) = scripts.get(&script_name) else { | ||
| return Ok(None); | ||
| }; | ||
|
|
||
| if scripts.contains_key(&format!("pre{script_name}")) | ||
| || scripts.contains_key(&format!("post{script_name}")) | ||
| || script_uses_node_run_unsupported_env(script) | ||
| || ctx.project_state()?.has_yarn_pnp_loader() | ||
| { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are other places in the codebase that handle project_state errors gracefully
rg -n "project_state\(\)" --type rust -A 2 | head -50Repository: happytoolin/hni
Length of output: 885
🏁 Script executed:
#!/bin/bash
# Find all callers of build_node_run_exec_if_safe
rg -n "build_node_run_exec_if_safe" --type rustRepository: happytoolin/hni
Length of output: 348
🏁 Script executed:
#!/bin/bash
# Check the return type and error handling of resolve_nr and resolve_node_run
rg -n "fn resolve_nr|fn resolve_node_run" --type rust -A 1Repository: happytoolin/hni
Length of output: 355
🏁 Script executed:
#!/bin/bash
# Look for how project_state errors are typically handled - check for .ok() or similar patterns
rg -n "project_state\(\)" --type rust -B 1 -A 3 | grep -A 3 "\.ok()" | head -30Repository: happytoolin/hni
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Read lines 120-135 to see the full context of the first call at line 127
sed -n '120,135p' src/core/resolve/build.rs | cat -nRepository: happytoolin/hni
Length of output: 853
🏁 Script executed:
#!/bin/bash
# Read lines 200-215 to see the full context of the second call at line 208
sed -n '200,215p' src/core/resolve/build.rs | cat -nRepository: happytoolin/hni
Length of output: 784
🏁 Script executed:
#!/bin/bash
# Find callers of resolve_nr and resolve_node_run to see how they handle HniResult errors
rg -n "resolve_nr\(|resolve_node_run\(" --type rust -B 2 -A 2 | head -60Repository: happytoolin/hni
Length of output: 3857
🏁 Script executed:
#!/bin/bash
# Check if there's error handling middleware or if errors are just bubbled up
rg -n "\.map_err|\.unwrap|match.*resolve_nr|match.*resolve_node_run" --type rust | head -20Repository: happytoolin/hni
Length of output: 1622
Error propagation in project_state() calls prevents graceful fallback.
The ? operators at lines 517 and 529 propagate errors from ctx.project_state() instead of returning Ok(None). If package.json is malformed, the entire resolve_nr() or resolve_node_run() call will error rather than falling back to package manager execution. This is inconsistent with the function's design, which handles multiple unsafe conditions by returning Ok(None). Consider converting project_state() errors to Ok(None) for a truly graceful fallback strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/resolve/build.rs` around lines 499 - 532, The calls to
ctx.project_state() currently use ? which propagates errors instead of falling
back; fetch project_state once and convert failures into Ok(None) (e.g., use a
match or if let to return Ok(None) on Err), then reuse that project_state for
nearest_package() and has_yarn_pnp_loader() checks; update
build_node_run_exec_if_safe to obtain project_state with error-to-Ok(None)
handling and then proceed with let Some(pkg) = project_state.nearest_package()
else return Ok(None) and use project_state.has_yarn_pnp_loader() where needed.
Comment on lines
+90
to
+113
| let deadline = Instant::now() + NODE_RUN_PROBE_TIMEOUT; | ||
| loop { | ||
| match child.try_wait() { | ||
| Ok(Some(_)) => break, | ||
| Ok(None) if Instant::now() < deadline => thread::sleep(Duration::from_millis(10)), | ||
| Ok(None) | Err(_) => { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut stdout = Vec::new(); | ||
| let mut stderr = Vec::new(); | ||
| if let Some(mut pipe) = child.stdout.take() { | ||
| let _ = pipe.read_to_end(&mut stdout); | ||
| } | ||
| if let Some(mut pipe) = child.stderr.take() { | ||
| let _ = pipe.read_to_end(&mut stderr); | ||
| } | ||
| let _ = child.wait(); | ||
|
|
||
| help_text_supports_run(&stdout) || help_text_supports_run(&stderr) |
There was a problem hiding this comment.
Treat failed --help probes as unsupported.
This probe currently trusts the output even when node --help exits non-zero. A wrapper or bad HNI_REAL_NODE override that prints --run in its error text will return true here and can steer normal run flows onto an unsupported node --run path. Check status.success() before inspecting stdout/stderr.
Suggested fix
- let deadline = Instant::now() + NODE_RUN_PROBE_TIMEOUT;
- loop {
+ let deadline = Instant::now() + NODE_RUN_PROBE_TIMEOUT;
+ let status = loop {
match child.try_wait() {
- Ok(Some(_)) => break,
+ Ok(Some(status)) => break status,
Ok(None) if Instant::now() < deadline => thread::sleep(Duration::from_millis(10)),
Ok(None) | Err(_) => {
let _ = child.kill();
let _ = child.wait();
return false;
}
}
}
+
+ if !status.success() {
+ return false;
+ }
let mut stdout = Vec::new();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/node.rs` around lines 90 - 113, The current probe inspects
stdout/stderr even when the child process exits with a non-zero status, which
can falsely detect support; modify the logic around the child process handling
in the node --help probe (the code that uses child.try_wait(), child.wait(), and
help_text_supports_run) to check the exit status (Status from child.wait()) and
if !status.success() return false (treat as unsupported) before calling
help_text_supports_run on stdout/stderr; ensure you still drain stdout/stderr
safely after waiting, but do not consider their contents unless status.success()
is true.
This was referenced Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nrandnlxnode --runfornode runwhen the installed Node supports it and the script is safe for that pathDetails
nronnative -> node --run -> package managernode runthroughnode --run -> native -> package manager--runcapability detection and surface it through doctor/explainTesting
nrandnode runfast paths versus--no-nativeSummary by CodeRabbit
New Features
Improvements
Chores