Skip to content

Commit bd0a60d

Browse files
committed
fix(shim): fix package binary detection for wrapper scripts
The is_potential_package_binary function was using current_exe() to determine if we're running from the configured bin directory. With wrapper scripts, current_exe() returns the wrapper's location (current/bin/vp-raw), not the original shim's location (bin/projj). Fix: Check if the shim exists in the configured bin directory directly instead of comparing current_exe()'s parent with the configured bin dir. Also add tracing::debug! calls for easier debugging of shim dispatch.
1 parent 45c45ce commit bd0a60d

3 files changed

Lines changed: 23 additions & 70 deletions

File tree

crates/vite_global_cli/src/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ async fn main() -> ExitCode {
4949
// Check for shim mode (invoked as node, npm, or npx)
5050
let args: Vec<String> = std::env::args().collect();
5151
let argv0 = args.first().map(|s| s.as_str()).unwrap_or("vp");
52+
tracing::debug!("argv0: {argv0}");
5253

5354
if let Some(tool) = shim::detect_shim_tool(argv0) {
5455
// Shim mode - dispatch to the appropriate tool

crates/vite_global_cli/src/shim/dispatch.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ const RECURSION_ENV_VAR: &str = "VITE_PLUS_TOOL_RECURSION";
2828
/// Called when the binary is invoked as node, npm, npx, or a package binary.
2929
/// Returns an exit code to be used with std::process::exit.
3030
pub async fn dispatch(tool: &str, args: &[String]) -> i32 {
31+
tracing::debug!("dispatch: tool: {tool}, args: {:?}", args);
3132
// Check recursion prevention - if already in a shim context, passthrough directly
3233
if std::env::var(RECURSION_ENV_VAR).is_ok() {
3334
return passthrough_to_system(tool, args);

crates/vite_global_cli/src/shim/mod.rs

Lines changed: 21 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,12 @@ pub fn is_shim_tool(tool: &str) -> bool {
6262

6363
/// Check if the tool could be a package binary shim.
6464
///
65-
/// Returns true if the tool is invoked from the vite-plus bin directory.
65+
/// Returns true if a shim for the tool exists in the configured bin directory.
6666
/// This check respects the VITE_PLUS_HOME environment variable for custom home directories.
67+
///
68+
/// Note: We check the configured bin directory directly instead of using current_exe()
69+
/// because when running through a wrapper script (e.g., current/bin/vp), the current_exe()
70+
/// returns the wrapper's location, not the original shim's location.
6771
fn is_potential_package_binary(tool: &str) -> bool {
6872
use crate::commands::env::config;
6973

@@ -72,33 +76,10 @@ fn is_potential_package_binary(tool: &str) -> bool {
7276
return false;
7377
};
7478

75-
// Check if we're running from the configured bin directory
76-
let Ok(current_exe) = std::env::current_exe() else {
77-
return false;
78-
};
79-
80-
let Some(bin_dir) = current_exe.parent() else {
81-
return false;
82-
};
83-
84-
// Compare the executable's bin directory with the configured bin directory
85-
// Use canonicalize to resolve symlinks and get consistent paths
86-
let bin_dir_canonical = std::fs::canonicalize(bin_dir).ok();
87-
let configured_canonical = std::fs::canonicalize(configured_bin.as_path()).ok();
88-
89-
let is_in_configured_bin = match (bin_dir_canonical, configured_canonical) {
90-
(Some(a), Some(b)) => a == b,
91-
// Fallback to direct comparison if canonicalize fails
92-
_ => bin_dir == configured_bin.as_path(),
93-
};
94-
95-
if !is_in_configured_bin {
96-
return false;
97-
}
98-
99-
// Check if the shim exists in the bin directory
100-
let shim_path = bin_dir.join(tool);
101-
shim_path.exists()
79+
// Check if the shim exists in the configured bin directory
80+
// Use symlink_metadata to detect symlinks (even broken ones)
81+
let shim_path = configured_bin.join(tool);
82+
std::fs::symlink_metadata(&shim_path).is_ok()
10283
}
10384

10485
/// Detect the shim tool from environment and argv.
@@ -166,50 +147,20 @@ mod tests {
166147
assert!(!is_shim_tool("vp")); // vp is never a shim
167148
}
168149

169-
/// Test that package binary detection works with custom VITE_PLUS_HOME.
170-
///
171-
/// BUG: Currently, is_potential_package_binary() uses a hardcoded string check:
172-
/// `bin_dir_str.contains(".vite-plus") && bin_dir_str.ends_with("bin")`
173-
///
174-
/// This fails when VITE_PLUS_HOME is set to a custom directory like
175-
/// "~/.vite-plus-dev" because ".vite-plus-dev" contains ".vite-plus" but
176-
/// is a different directory, or when set to something like "~/.my-tools"
177-
/// which doesn't contain ".vite-plus" at all.
150+
/// Test that is_potential_package_binary checks the configured bin directory.
178151
///
179-
/// The fix is to use config::get_bin_dir() which respects VITE_PLUS_HOME.
152+
/// The function now checks if a shim exists in the configured bin directory
153+
/// (from VITE_PLUS_HOME/bin) instead of relying on current_exe().
154+
/// This allows it to work correctly with wrapper scripts.
180155
#[test]
181-
fn test_is_potential_package_binary_with_custom_home_conceptual() {
182-
// This is a conceptual test that documents the bug.
183-
// We can't easily test the actual function because it relies on
184-
// std::env::current_exe() which we can't mock.
156+
fn test_is_potential_package_binary_checks_configured_bin() {
157+
// The function checks config::get_bin_dir() which respects VITE_PLUS_HOME.
158+
// Without setting VITE_PLUS_HOME, it defaults to ~/.vite-plus/bin.
185159
//
186-
// The bug is that this check:
187-
// bin_dir_str.contains(".vite-plus") && bin_dir_str.ends_with("bin")
188-
//
189-
// Would fail for these valid VITE_PLUS_HOME values:
190-
// - ~/.my-node-manager (doesn't contain ".vite-plus")
191-
// - /opt/vp (doesn't contain ".vite-plus")
192-
//
193-
// And incorrectly match:
194-
// - ~/.vite-plus-dev (contains ".vite-plus" but is a different dir)
195-
//
196-
// After the fix, we compare against config::get_bin_dir() directly.
197-
198-
// Test the bug exists in the current implementation by checking the string logic
199-
let cases = [
200-
// (bin_dir, expected_with_bug, expected_after_fix)
201-
("/home/user/.vite-plus/bin", true, true), // Normal case
202-
("/home/user/.vite-plus-dev/bin", true, false), // BUG: matches but shouldn't
203-
("/home/user/.my-tools/bin", false, true), // BUG: doesn't match but should
204-
("/opt/vp/bin", false, true), // BUG: doesn't match but should
205-
];
206-
207-
for (bin_dir, expected_with_bug, _expected_after_fix) in cases {
208-
let result_with_bug = bin_dir.contains(".vite-plus") && bin_dir.ends_with("bin");
209-
assert_eq!(result_with_bug, expected_with_bug, "Bug check failed for {bin_dir}");
210-
}
211-
212-
// The fix will replace string matching with path comparison
213-
// using config::get_bin_dir() which respects VITE_PLUS_HOME env var
160+
// Since we can't easily create test shims in the actual bin directory,
161+
// we just verify the function doesn't panic and returns false for
162+
// non-existent tools.
163+
assert!(!is_potential_package_binary("nonexistent-tool-12345"));
164+
assert!(!is_potential_package_binary("another-fake-tool"));
214165
}
215166
}

0 commit comments

Comments
 (0)