Add Nushell support#9393
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I requested changes on this pull request and posted feedback. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds local Nushell support across shell detection, bootstrap scripts, command execution, environment-variable serialization, and related shell metadata.
Concerns
- Linux update command construction treats Nushell's success-gated combiner as a plain separator, so update completion can be reported even after package-manager failures.
- Drive env-var export/copy paths still convert a
ShellFamily::Posixvalue back toShellType::Bash, which loses Nushell context and emits Bash syntax for Nu users. - Nushell detection uses broad substring/suffix checks that can misclassify non-Nu executables as Nushell, especially because
nuis only two characters.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // Nushell does not support POSIX `&&`. There is no direct command separator with the | ||
| // same status-gated semantics, so use a plain separator as the least surprising | ||
| // fallback for command construction sites that are not Nushell-specific. | ||
| ShellType::Nu => "; ", |
There was a problem hiding this comment.
and_combiner() is used by the Linux update flow before warp_finish_update; returning a plain ; makes Nushell report the update as finished even when the package-manager command fails. Keep Nu out of status-gated update command construction or use a Nu expression that preserves the success dependency.
There was a problem hiding this comment.
Fixed in 9a654d8: Nu update commands now use a dedicated nu_update_command that wraps package-manager steps and warp_finish_update in one Nushell try/catch block, so finish is not sent after a failed package-manager command. Added regression coverage in test_nu_update_command_gates_finish_update_on_success.
| let exported_variables = env_var_collection_model | ||
| .string_model | ||
| .export_variables("\n", shell_family) | ||
| .export_variables("\n", shell_family.into()) |
There was a problem hiding this comment.
ShellFamily::Posix back to ShellType::Bash loses Nushell context, so exported env-var collections still emit Bash syntax for Nu users. Carry the actual ShellType through this path or add an explicit Nushell export mode.
There was a problem hiding this comment.
Fixed in 9a654d8: Drive export and copy now carry the active session ShellType from active_session_shell_type instead of round-tripping through ShellFamily::Posix -> Bash, so Nu sessions use the Nushell env-var serializer.
| Some(ShellType::Zsh) | ||
| } else if name == "fish" || name == "-fish" || name.ends_with("/fish") { | ||
| Some(ShellType::Fish) | ||
| } else if name == "nu" || name == "-nu" || name.ends_with("/nu") || name.ends_with("nu.exe") |
There was a problem hiding this comment.
name.ends_with("nu.exe") also matches executables like menu.exe, causing unrelated commands to be detected as Nushell. Compare the basename to nu.exe exactly instead of using a suffix match for this short name.
There was a problem hiding this comment.
Fixed in 9a654d8: ShellType::from_name now compares the executable basename to nu.exe exactly; added regression coverage that menu.exe is not detected as Nu.
| ShellType::Zsh | ||
| } else if shell_path.contains("fish") { | ||
| ShellType::Fish | ||
| } else if shell_path.contains("nu") { |
There was a problem hiding this comment.
shell_path.contains("nu") is too broad for WSL default-shell detection and can classify any path containing nu as Nushell. Parse the basename with ShellType::from_name or compare the executable name exactly.
There was a problem hiding this comment.
Fixed in 9a654d8: WSL default-shell detection now extracts the basename and delegates to ShellType::from_name; added regression coverage for /usr/bin/menu.exe.
Co-Authored-By: Warp <agent@warp.dev>
|
I'm checking this implementation PR for association with an explicitly linked issue. The PR that you've opened seems to contain implementation changes and is associated with issue #2038, but none of those associated issue are marked as Powered by Oz |
|
Thanks for taking a look here. Quick update: I realized this implementation PR got ahead of the repo flow — #2038 is still labeled I’ve opened the required spec-only PR here: #9470. I’ve also been tightening that spec based on Oz’s feedback, especially around the Nushell startup contract, metadata payloads, env-var escaping, update-command failure semantics, and reproducible validation. Once the spec path is approved and the issue is marked ready for implementation, I’m happy to continue from this branch and keep iterating on the implementation. I care about landing this with good shell-integration behavior and solid tests rather than just making |
1 participant
Description
Fixes #2038.
Adds local Nushell (
nu) support across Warp's shell model, discovery, launch, bootstrap, command execution, and env-var serialization paths.ShellType::Nudetection fornu,nu.exe, markdownnu/nushell, history/rc files, alias parsing, executable discovery, and shell metadata.Testing
cargo fmt --all -- --checkexport PROTOC=$(nix-shell -p protobuf --run 'which protoc'); cargo check -p warpcargo test -p warp env_vars::tests --libcargo test -p warp test_find_by_command_name_matches_known_shell --libcargo test -p warp test_trims_comments --libcargo test -p warp validate_local_harness_shell_rejects_unsupported_shells --libcargo test -p warp_terminal test_from_name --libcargo test -p warp_terminal test_from_markdown_language_spec --libcargo test -p warp_terminal test_nu_parse_aliases --libnix develop /home/vitalyr/projects/dev/ai/warp#default -c bash -lc 'cd /home/vitalyr/projects/dev/ai/warp-dev && cargo test -p warp_terminal test_nu_parse_aliases --lib'export PROTOC=$(nix-shell -p protobuf --run 'which protoc'); export LIBCLANG_PATH=$(nix eval --raw nixpkgs#llvmPackages.libclang.lib)/lib; cargo clippy -p warp -p warp_terminal --all-targets --tests -- -D warningsnu -n --no-std-libsourcing the generated bootstrap files.echo hi.Note: attempted full workspace clippy locally, but this shell lacked
pkg-configforalsa-sys; targeted clippy for touched crates passed and GitHub CI should run the full check in the proper environment.Server API dependencies
None.
Agent Mode
Changelog Entries for Stable
CHANGELOG-NEW-FEATURE: Added Nushell as a supported local shell.
Co-Authored-By: Warp agent@warp.dev