feat(env): warn when PATH is long enough for cmd.exe to ignore - #11643
Conversation
📝 WalkthroughWalkthroughThe change detects Windows ChangesPATH limit warning
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/path_env.rs (1)
185-202: 🧹 Nitpick | 🔵 TrivialAlign the warning message with the current lifetime.
warn_once!keys on the fully interpolatedmsg, andwarn_if_cmd_ignores_path_str()is called byhook-env, which runs as a new process on each shell render. If the process starts once per prompt, change the doc comment from “warn once per process” to “warn once per invocation” unless the behavior is changed to persist the coarser key across activation iterations.If
PATHlengths change between thePathEnvjoin and the manual hook PATH, the warning can still happen twice in the same process because the interpolated message differs. Use a stable key if that deduplication is intended.🤖 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 `@src/path_env.rs` around lines 185 - 202, Update the documentation above warn_if_cmd_ignores_path to say the warning occurs once per invocation, matching hook-env’s process lifetime. In warn_if_cmd_ignores_path, use a stable warn_once! deduplication key rather than the fully interpolated message if warnings must remain deduplicated when PATH lengths differ within one process; preserve the existing diagnostic text and length values.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@src/path_env.rs`:
- Around line 185-202: Update the documentation above warn_if_cmd_ignores_path
to say the warning occurs once per invocation, matching hook-env’s process
lifetime. In warn_if_cmd_ignores_path, use a stable warn_once! deduplication key
rather than the fully interpolated message if warnings must remain deduplicated
when PATH lengths differ within one process; preserve the existing diagnostic
text and length values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f0b16f48-4d7c-4f3b-9fb8-e8bc3d21a156
📒 Files selected for processing (2)
src/cli/hook_env.rssrc/path_env.rs
ca93282 to
f95c26a
Compare
Past 8191 characters cmd.exe drops an inherited environment variable entirely rather than truncating it, so every command run through it reports "is not recognized" with nothing naming PATH, cmd.exe or mise. Checked on both surfaces that can reach that length: PathEnv::join, which every computed child environment goes through, and hook-env's hand-assembled PATH, which is the copy a tool started directly from an activated shell inherits. The predicate is pure and counts UTF-16 code units the way Windows does, so the boundary is unit-testable on every platform and cannot fire off Windows. This only reports the condition — it does not lower the ceiling. Reported in jdx#5830
f95c26a to
ab5ab87
Compare
|
Both halves taken. The double warning is real, and it is the common path rather than an edge case. One static WARNED_CMD_PATH_LIMIT: AtomicBool = AtomicBool::new(false);
...
if WARNED_CMD_PATH_LIMIT.swap(true, Ordering::Relaxed) {
return;
}That removes the first of the open questions in the description, so I have edited it out. "once per process" was accurate and reassuring in the wrong way, since under
That cadence is still the thing I most want a decision on — the guard above makes it one warning per prompt instead of two, which is an improvement rather than an answer. While in there I also corrected the constant's doc comment, which claimed "nothing on PATH resolves and every command reports is not recognized". Programs in the system directory keep working, because |
Greptile SummaryAdds Windows-specific diagnostics when a generated PATH exceeds cmd.exe's 8191 UTF-16-code-unit limit.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure identified. The warning is Windows-gated, measures the relevant UTF-16 length, covers both PATH construction paths, and remains separate from the shell commands emitted on stdout. Important Files Changed
Reviews (1): Last reviewed commit: "feat(env): warn when PATH is long enough..." | Re-trigger Greptile |
Draft — the detection is straightforward, but where it should fire and how loudly is your call. #5830's fifth suggestion, and the only one of that thread's eight that does not need a design decision first.
What breaks, measured
Past 8191 characters
cmd.exedoes not truncate an inherited variable — it drops it. Nothing on PATH resolves, sonpm,npxand batch scripts report that every command is unrecognised, with nothing naming PATH, cmd.exe or mise.Windows 11 26200 / cmd 10.0.26100.8875, same PATH for both columns,
Git\cmdappended last:where.exe wheregit --version(The
where.execolumn is there because it is the probedocs/troubleshooting.mdcurrently recommends, and it never discriminates —where.exeis in System32, which cmd.exe resolves while ignoring PATH. #11642 fixes that separately.)KB 830473 documents the behaviour, but its "Applies to" stops at Windows 7 / Server 2008 R2 / 2012 R2 and hedges with "as appropriate to the operating system" — hence the measurement above rather than a citation alone.
Where it fires
Two surfaces can reach that length, and they do not share a code path:
PathEnv::join— every environment mise computes:mise x/runchildren,mise env, backends, core plugins.hook_env.rs— assembles the shell's PATH by hand withjoin_paths(pre + user_paths + tool_paths + post + post_user), soPathEnv::join's check never sees it. This is the copy a tool started directly from an activated shell inherits, which is the reported symptom (npm installfrom a normal prompt), so leaving it out would miss the main case.warn_once!rather thanhint!: hints requireconsole::user_attended()and so never appear in scripts or CI, and are suppressed permanently after one appearance. A broken environment should be reported every process.Correctness details
windows-unitjob, andcfg!(windows)inside it means it can never fire elsewhere.OsStr::lenwould return WTF-8 bytes and treat a three-byte character as three, warning about a PATH cmd.exe accepts. Pinned by a test.This only reports the condition. It does not shorten anything — a large enough toolset still hits the limit, and the structural proposals in #5830 (shorter install dirs, hashed paths, a per-project shim dir) are untouched.
Open questions — why this is a draft
disable_hints-style setting?CreateProcessfails outright with "not enough memory resources" — also measured. Louder failure, equally opaque cause. Worth catching here too, or out of scope?e2e-win/run.ps1(build a long PATH, runmise x, assert the warning) — happy to add it once the above are settled, rather than write it against a shape that may change.Summary by CodeRabbit