Skip to content

Commit 0f9c7f0

Browse files
committed
fix: address second-round CodeRabbit review on PR #6
- network: scope remote_cache filename per target triple so a shared PVM_DIR (NFS, dotfile sync) cannot serve linux-x86_64 entries to an aarch64 host - use_cmd, install: remove unsafe std::env::set_var of PATH and PVM_MULTISHELL_PATH; the wrapper sources env_file into the parent shell on exit, and set_var is unsound under the multi-threaded tokio runtime - ls_remote: hoist 'prefix.' allocation outside retain so the predicate does not reallocate per element - docs: fix GEMINI.md PVM_DIR default for macOS, env file naming, and the data-integrity guideline to point at std::fs::File locking (fs4 was dropped in an earlier commit)
1 parent e6130ed commit 0f9c7f0

5 files changed

Lines changed: 20 additions & 34 deletions

File tree

GEMINI.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,11 @@ When running `git commit -m "..."` in the shell (like Zsh/Bash), backticks are i
1313

1414
## Project Architecture (The Map)
1515
### Filesystem Hierarchy
16-
- **$PVM_DIR**: Root directory, defaults to `~/.local/share/pvm` on Linux/macOS.
16+
- **$PVM_DIR**: Root directory. Resolved via `dirs::data_local_dir()`, so defaults to `~/.local/share/pvm` on Linux and `~/Library/Application Support/pvm` on macOS.
1717
- **$PVM_DIR/versions/<version>**: Installation directory for specific PHP versions.
1818
- **$PVM_DIR/bin/pvm**: The `pvm` binary itself.
19-
- **$PVM_DIR/remote_cache.json**: 24-hour cache for remote version data.
20-
- **$PVM_DIR/.env**: The generated file containing active version exports.
19+
- **$PVM_DIR/remote_cache-<target-triple>.json**: 24-hour cache for the remote version index, scoped per target triple (e.g. `linux-x86_64`).
20+
- **$PVM_DIR/.env_update[_<shell-pid>]**: Short-lived files written per shell invocation; the shell wrapper sources them to mutate the parent shell's environment.
2121

2222
### Module Responsibilities
2323
- `src/cli.rs`: Command definitions using `clap`.
@@ -46,7 +46,7 @@ When running `git commit -m "..."` in the shell (like Zsh/Bash), backticks are i
4646
- **Interactivity:** Use `dialoguer` for menus and confirmations.
4747
- **Icons:** Use `colored` for status icons: `` (green), `` (red), `` (blue), `💡` (yellow).
4848
- **Async:** Use `tokio` for runtime and `reqwest` for all network I/O.
49-
- **Data Integrity:** Use `fs4` file locking when writing to `.env` or `remote_cache.json`.
49+
- **Data Integrity:** Use file locking (`std::fs::File::lock` / `lock_shared` / `unlock`, stable since Rust 1.89) when writing to env update files or the remote cache.
5050

5151
## Testing & Validation
5252
### Testing Protocol

src/commands/install.rs

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -116,19 +116,9 @@ pub async fn execute_install(version: &str) -> Result<()> {
116116
let env_file = crate::fs::get_env_update_path(None)?;
117117
crate::fs::write_env_file_locked(&env_file, &format!("{}\n{}", export_str1, export_str2))?;
118118

119-
unsafe {
120-
std::env::set_var(MULTISHELL_PATH_VAR, &bin_dir);
121-
if let Some(path) = std::env::var_os("PATH") {
122-
let mut new_path = std::ffi::OsString::new();
123-
new_path.push(&bin_dir);
124-
#[cfg(windows)]
125-
new_path.push(";");
126-
#[cfg(not(windows))]
127-
new_path.push(":");
128-
new_path.push(&path);
129-
std::env::set_var("PATH", new_path);
130-
}
131-
}
119+
// Note: process-global env is intentionally NOT mutated here. std::env::set_var
120+
// is unsound in a multi-threaded tokio runtime, and the wrapper sources env_file
121+
// into the parent shell on exit, so subsequent pvm invocations see the new PATH.
132122
println!("{} Switched to PHP {}", "✓".green(), v.bold());
133123
} else if !cli_selected {
134124
println!(

src/commands/ls_remote.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ impl LsRemote {
1616
let mut versions_info = network::get_available_versions().await?;
1717

1818
if let Some(prefix) = &self.version_prefix {
19-
versions_info.retain(|(v, _)| v == prefix || v.starts_with(&format!("{}.", prefix)));
19+
let prefix_dot = format!("{}.", prefix);
20+
versions_info.retain(|(v, _)| v == prefix || v.starts_with(&prefix_dot));
2021
}
2122

2223
if versions_info.is_empty() {

src/commands/use_cmd.rs

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -108,20 +108,9 @@ impl Use {
108108
let env_file = fs::get_env_update_path(None)?;
109109
fs::write_env_file_locked(&env_file, &format!("{}\n{}", export_str1, export_str2))?;
110110

111-
// Also update the current Rust binary's environment so spawned subs (or interactive loop) see it
112-
unsafe {
113-
std::env::set_var(MULTISHELL_PATH_VAR, &bin_dir);
114-
if let Some(path) = std::env::var_os("PATH") {
115-
let mut new_path = std::ffi::OsString::new();
116-
new_path.push(&bin_dir);
117-
#[cfg(windows)]
118-
new_path.push(";");
119-
#[cfg(not(windows))]
120-
new_path.push(":");
121-
new_path.push(&path);
122-
std::env::set_var("PATH", new_path);
123-
}
124-
}
111+
// Note: process-global env is intentionally NOT mutated here. std::env::set_var
112+
// is unsound in a multi-threaded tokio runtime, and the wrapper sources env_file
113+
// into the parent shell on exit, so subsequent pvm invocations see the new PATH.
125114

126115
Ok(())
127116
}

src/network.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,14 @@ fn get_target_triple() -> Result<&'static str> {
4545

4646
pub async fn get_available_versions() -> Result<Vec<(String, Vec<String>)>> {
4747
let pvm_dir = crate::fs::get_pvm_dir()?;
48-
let cache_path = pvm_dir.join(REMOTE_CACHE_FILE);
48+
let target = get_target_triple()?;
49+
// Cache is filtered by target triple, so the filename must include it
50+
// to prevent cross-arch reuse when $PVM_DIR is shared (e.g. via NFS).
51+
let cache_path = pvm_dir.join(format!(
52+
"{}-{}.json",
53+
REMOTE_CACHE_FILE.trim_end_matches(".json"),
54+
target
55+
));
4956

5057
// 1. Try to load from valid cache
5158
if cache_path.exists()
@@ -96,7 +103,6 @@ pub async fn get_available_versions() -> Result<Vec<(String, Vec<String>)>> {
96103
.context("Failed to parse remote version JSON")?;
97104
spinner.finish_and_clear();
98105

99-
let target = get_target_triple()?;
100106
let suffix = format!("-{}.tar.gz", target);
101107

102108
let mut versions_map: std::collections::HashMap<String, Vec<String>> =

0 commit comments

Comments
 (0)