Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cmd/activation_switch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ pub fn sync_profile_after_user_deactivate(
}

/// Keep `nu_state/paths.json` aligned with the newly selected active Nu.
fn refresh_cached_nu_paths_after_switch(root: &Path) -> Result<()> {
pub(crate) fn refresh_cached_nu_paths_after_switch(root: &Path) -> Result<()> {
let paths_file = root.join("nu_state").join("paths.json");
if !paths_file.is_file() {
return Ok(());
Expand Down
109 changes: 98 additions & 11 deletions src/cmd/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::nu::bootstrap::managed_nu_binary;
use crate::nu::paths::{
discover_nu_off_path, find_nu_executable_with_root, find_nu_on_path, NuPaths,
};
use crate::nu::version_manager;
use crate::nu::version_manager::{self, VersionManagerError};
use crate::nupm_compat::NupmCompatibility;
use crate::nupm_compat::{
count_drifted_imports, resolve_nupm_home, scan_nupm_home, NupmHomeResolution,
Expand Down Expand Up @@ -517,33 +517,68 @@ fn check_nu_environments(root: &Path, options: &DoctorOptions, findings: &mut Ve
None,
RepairTier::None,
)),
Err(e) => findings.push(finding(
Err(e) => findings.push(managed_nu_resolve_finding(e)),
}
}

/// Map managed-Nu resolution failures to actionable doctor findings.
///
/// `CMD_USE` is only appropriate for a dangling active-version marker.
/// Filesystem scan / marker parse failures need a different message and no
/// `numan use` hint.
fn managed_nu_resolve_finding(err: VersionManagerError) -> Finding {
match &err {
VersionManagerError::DanglingActive { .. }
| VersionManagerError::DanglingActiveWithOffTree { .. } => finding(
"nu.managed.version",
Severity::Warn,
format!("Managed Nu: could not resolve active managed binary ({e})"),
format!("Managed Nu: could not resolve active managed binary ({err})"),
Some(CMD_USE),
RepairTier::Manual,
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
)),
),
VersionManagerError::ReadVersionsDir { .. }
| VersionManagerError::ReadLegacyVersion { .. }
| VersionManagerError::ReadMarker { .. }
| VersionManagerError::MalformedMarker { .. }
| VersionManagerError::InvalidVersion { .. } => finding(
"nu.managed.version",
Severity::Warn,
format!(
"Managed Nu: could not resolve managed Nu binary ({err}). \
Check permissions on tools/nushell and nu_state, or reinstall with \
`{CMD_SETUP_NU}`."
),
None,
RepairTier::Manual,
),
_ => finding(
"nu.managed.version",
Severity::Warn,
format!("Managed Nu: could not resolve managed Nu binary ({err})"),
None,
RepairTier::Manual,
),
}
}

fn resolve_managed_nu_binary(root: &Path) -> Result<Option<PathBuf>> {
/// Resolve the managed Nu binary for doctor reporting.
///
/// Prefers the versioned active install (`tools/nushell/<version>/nu`), then
/// falls back to the legacy single-binary path (`tools/nushell/nu`) so older
/// roots still report correctly before migration.
fn resolve_managed_nu_binary(root: &Path) -> Result<Option<PathBuf>, VersionManagerError> {
match version_manager::active_nu_binary(root) {
Ok(Some(path)) => return Ok(Some(path)),
Ok(None) => {}
Err(e) => {
return Err(e).with_context(|| "Failed to resolve active managed Nu binary");
}
Err(e) => return Err(e),
}

let legacy = managed_nu_binary(root);
if legacy.is_file() {
return Ok(Some(legacy));
}

if let Some(latest) = version_manager::latest_installed_version(root)
.with_context(|| "Failed to list installed managed Nu versions")?
{
if let Some(latest) = version_manager::latest_installed_version(root)? {
let path = version_manager::version_binary(root, &latest);
if path.is_file() {
return Ok(Some(path));
Expand Down Expand Up @@ -2401,13 +2436,65 @@ mod tests {
.expect("nu.managed.version");
assert_eq!(managed.severity, Severity::Warn);
assert_eq!(managed.repair, RepairTier::Manual);
assert_eq!(managed.fix.as_deref(), Some(CMD_USE));
assert!(
managed.message.contains("active managed binary"),
"dangling active should keep active-binary wording: {}",
managed.message
);
assert!(
!managed.message.contains("not installed"),
"should not report 'not installed' when marker exists: {}",
managed.message
);
}

#[test]
fn doctor_reports_managed_nu_scan_failure_without_use_hint() {
let dir = TempDir::new().unwrap();
let root = dir.path();
// Make tools/nushell a file so read_dir fails with ReadVersionsDir.
let tools = root.join("tools");
std::fs::create_dir_all(&tools).unwrap();
std::fs::write(tools.join("nushell"), b"not-a-directory").unwrap();

let report = run_checks_with_options(
&DoctorArgs {
scan: true,
json: true,
nupm_home: None,
},
root,
&test_doctor_options(),
)
.unwrap();

let managed = report
.findings
.iter()
.find(|f| f.id == "nu.managed.version")
.expect("nu.managed.version");
assert_eq!(managed.severity, Severity::Warn);
assert_eq!(managed.repair, RepairTier::Manual);
assert!(
managed.fix.is_none(),
"filesystem scan failures must not hint `{CMD_USE}`: {:?}",
managed.fix
);
assert!(
managed
.message
.contains("could not resolve managed Nu binary"),
"expected generic resolve wording: {}",
managed.message
);
assert!(
!managed.message.contains("active managed binary"),
"scan failure must not use dangling-active wording: {}",
managed.message
);
}

#[test]
fn doctor_reports_managed_nu_not_installed() {
let dir = TempDir::new().unwrap();
Expand Down
3 changes: 3 additions & 0 deletions src/cmd/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ fn list_packages(root: &Path) -> Result<()> {
Ok(())
}

/// Convert terminal column count to usable description width.
fn terminal_cols_to_description_width(terminal_cols: usize) -> usize {
let available = terminal_cols.saturating_sub(4);
if available >= 40 {
Expand All @@ -216,6 +217,7 @@ fn terminal_cols_to_description_width(terminal_cols: usize) -> usize {
}
}

/// Usable width for indented package descriptions (leave room for ` ` prefix).
fn package_description_width() -> usize {
let cols = console::Term::stdout()
.size_checked()
Expand All @@ -224,6 +226,7 @@ fn package_description_width() -> usize {
terminal_cols_to_description_width(cols)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// Soft-wrap `text` on whitespace so the terminal does not split mid-word.
fn wrap_words(text: &str, width: usize) -> Vec<String> {
if width == 0 {
return vec![text.to_string()];
Expand Down
26 changes: 13 additions & 13 deletions src/cmd/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,19 @@ fn execute_with_tty(args: &RemoveArgs, root: &Path, is_tty: bool) -> Result<()>
};
journal.save(root)?;

// Clear desire before any destructive change so a profile-write failure
// aborts the remove while the lockfile and payload are still intact and
// the user can retry. A leftover profile entry would make later
// `numan use` restore attempts target a removed package.
crate::state::activation_profile::remove_from_all_minors(root, &args.package).with_context(
|| {
format!(
"Failed to clear activation profile entries for '{}'",
args.package
)
},
)?;

// Remove from lockfile (atomic write).
lockfile.packages.remove(&args.package);
lockfile.save(root)?;
Expand Down Expand Up @@ -159,20 +172,7 @@ fn execute_with_tty(args: &RemoveArgs, root: &Path, is_tty: bool) -> Result<()>
}
}

// Clear desire while the remove journal is still present. Failure here
// must not report success: a leftover profile entry would make later
// `numan use` restore attempts target a removed package.
crate::state::activation_profile::remove_from_all_minors(root, &args.package).with_context(
|| {
format!(
"Failed to clear activation profile entries for '{}'",
args.package
)
},
)?;

PendingLifecycle::clear(root)?;

println!("{} Removed {}", console::style("✓").green(), args.package);

Ok(())
Expand Down
48 changes: 48 additions & 0 deletions src/cmd/use_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ fn execute_latest(
if let Some(path) = existing.binary_path.as_deref() {
if !std::path::Path::new(path).is_file() {
version_manager::write_active_version(root, &version)?;
Comment thread
tonythethompson marked this conversation as resolved.
// The cached paths.json still names the now-missing off-tree
// binary; reconcile_target_profile would load it and fail
// validate_drift after the marker has already changed.
activation_switch::refresh_cached_nu_paths_after_switch(root)?;
}
}
}
Expand Down Expand Up @@ -369,6 +373,50 @@ mod tests {
);
}

#[test]
fn test_use_latest_self_heal_clears_stale_paths_cache() {
let tmp = TempDir::new().unwrap();
let root = tmp.path();
create_fake_version(root, "0.113.1");
version_manager::write_active_version_with_binary(
root,
"0.113.1",
std::path::Path::new("/nonexistent/nu"),
)
.unwrap();

// Seed a paths.json that still points at the now-missing off-tree
// binary. Without a refresh after the marker repair, the same-target
// reconcile would load this cache and fail validate_drift.
let stale = NuPaths {
nu_executable: "/nonexistent/nu".to_string(),
nu_version: "0.113.1".to_string(),
plugin_registry_path: root.join("plugins.msgpackz").to_string_lossy().into_owned(),
nu_executable_hash: "deadbeef".to_string(),
platform: "test".to_string(),
data_dir: None,
vendor_autoload_dirs: vec![],
vendor_autoload_dir: None,
};
stale.save(root).unwrap();
assert!(root.join("nu_state/paths.json").is_file());

execute(
&UseArgs {
version: "latest".to_string(),
},
root,
)
.unwrap();

// The stale cache must be cleared so the repaired marker is the source
// of truth, not the dangling off-tree path.
assert!(
!root.join("nu_state/paths.json").exists(),
"stale paths.json must be cleared after dangling binary_path repair"
);
}

#[test]
fn test_use_latest_preserves_live_offtree_binary_path() {
let tmp = TempDir::new().unwrap();
Expand Down
Loading