Spec: Nushell support#9470
Conversation
Co-Authored-By: Warp <agent@warp.dev>
|
I ran into an unexpected error while working on this. I'm re-reviewing this spec-only pull request in response to a review request. I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
|
/oz-review |
![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 spec PR adds product and technical coverage for first-class local Nushell support, including shell detection, spawning, bootstrap assets, env-var serialization, Drive export behavior, update command construction, and follow-up scope.
Concerns
- The startup contract does not pin the exact Nushell invocation or config ordering needed to make local sessions both bootstrapped and interactive.
- The bootstrap section does not specify how Warp coexists with existing Nushell hooks, prompt config, keybindings, or config-load failures.
- The env-var serialization design needs explicit escaping rules and trust boundaries for values, commands, secrets, and quoted names to avoid invalid or injectable generated Nushell.
- Compatibility is underdefined because the spec does not set a minimum Nushell version or matrix for required flags, hooks, and syntax.
- The validation plan should distinguish rendered-script smoke checks from real interactive lifecycle acceptance tests.
Verdict
Found: 0 critical, 4 important, 1 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
| 2. Add local shell discovery and spawn support. | ||
| - Include `nu`/`nu.exe` in shell discovery and display it as "Nushell". | ||
| - Add `/bin/nu` fallback on Unix-like systems. | ||
| - Start direct Nushell sessions with login/execute arguments that run the Warp init script and then enter the interactive shell. |
There was a problem hiding this comment.
env.nu/config.nu/login.nu run before or after Warp; login/execute arguments is not enough for implementation or testability.
There was a problem hiding this comment.
Fixed in f5d0b89: the tech spec now pins the direct startup invocation as nu --login --execute , documents env.nu/config.nu/login.nu running before Warp init, and defines behavior for user config startup failures.
|
|
||
| 3. Add Nushell bootstrap assets. | ||
| - `nu_init_shell.nu` sends the initial `InitShell` hook and establishes `WARP_SESSION_ID`. | ||
| - `nu_body.nu` installs Nushell functions/hooks for `Preexec`, `Precmd`, `CommandFinished`, `Bootstrapped`, `Clear`, `InputBuffer`, `FinishUpdate`, prompt-mode toggles, PATH append, and initial working directory handling. |
There was a problem hiding this comment.
$env.config.hooks, prompt settings, keybindings, and user config errors; replacing or assuming empty config can break existing Nushell setups and makes the prompt-mode requirements ambiguous.
There was a problem hiding this comment.
Fixed in f5d0b89: the bootstrap section now specifies hook prepending, keybinding appending, prompt preservation/restoration, OSC shell-integration settings, and user config error handling.
| - Preserve command escaping rules separately from Bash/Zsh/Fish where needed. | ||
| - Route unsupported local child/subshell flows through the existing unsupported-shell handling instead of pretending Nushell is Bash. | ||
|
|
||
| 5. Add Nushell environment-variable serialization. |
There was a problem hiding this comment.
$(), or semicolons.
There was a problem hiding this comment.
Fixed in f5d0b89: env-var serialization now has explicit rules for JSON-escaped constants and names, command-backed values as trusted executable snippets, and secret-backed command substitutions with the secret manager reference as the trust boundary.
|
|
||
| - Behavior 1, 2, and 5: unit tests in `crates/warp_terminal/src/shell/mod_tests.rs` and `app/src/terminal/local_tty/shell_tests.rs` verify `nu`, `-nu`, `/usr/bin/nu`, Windows `nu.exe`, and false positives such as `menu.exe`/`/usr/bin/menu.exe`. | ||
| - Behavior 3 and 14: existing shell-discovery tests are extended so Nushell appears with the known shell types without regressing other shells. | ||
| - Behavior 4, 6, 7, 8, 9, and 10: bootstrap unit coverage verifies Nushell asset selection and local smoke testing runs the rendered Nushell init/body scripts with `nu`. |
There was a problem hiding this comment.
💡 [SUGGESTION] Split bootstrap validation from interactive lifecycle acceptance tests; the listed smoke checks do not prove spawn args, prompt toggles, command boundaries, and prompt redraws work inside a real interactive session.
There was a problem hiding this comment.
Fixed in f5d0b89: validation now separates rendered-script smoke checks from interactive lifecycle acceptance validation for spawn args, command boundaries, prompt toggles, hooks, keybindings, Drive export, and update completion behavior.
|
|
||
| ## Risks and mitigations | ||
|
|
||
| - **Nushell syntax changes over time.** Nushell is still evolving, so the bootstrap sticks to stable, simple constructs where possible and is covered by smoke tests against the local `nu` binary. |
There was a problem hiding this comment.
nu binary does not define behavior for older installed versions.
There was a problem hiding this comment.
Fixed in f5d0b89: added a compatibility section defining 0.109.x as the first supported Nushell line and documenting the flags, hooks, host commands, and try/catch behavior the bootstrap relies on.
Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Overview
This spec PR adds product and technical specs for first-class local Nushell support, covering shell detection, local spawning, bootstrap assets, env-var serialization, Drive export syntax, and Linux updater command construction.
Concerns
- The tech spec does not define the metadata discovery contract needed for Behavior 8, leaving custom commands, built-ins, keywords, env-var names, and related payload formats ambiguous.
- The updater plan relies on a Nushell
tryblock but does not specify the minimum Nushell version or exact failure-propagation behavior across package-manager command sequences.
Verdict
Found: 0 critical, 2 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
| - Parse markdown language specs `nu` and `nushell` as Nushell. | ||
| - Keep `ShellFamily::from(ShellType::Nu)` as `Posix` only for legacy family-level behavior, but avoid converting that family back to Bash in Nushell-specific export paths. | ||
| - Add Nushell history and rcfile locations. | ||
| - Parse Nushell aliases from tab-separated bootstrap output. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 5c79bf4: the tech spec now defines the Bootstrapped metadata contract for aliases, custom commands, built-ins, keywords, env-var names, PATH normalization, shell_path, and related metadata fields, plus validation coverage for those payloads.
| 5. Add Nushell environment-variable serialization. | ||
| - Constants serialize as JSON string literals assigned to `$env`, using `serde_json::to_string`. This makes quotes, backslashes, newlines, `$()`, semicolons, and other shell-significant characters data rather than executable Nushell syntax. | ||
| - Bare environment names are allowed only when they match `[A-Za-z_][A-Za-z0-9_]*`. Any other name serializes through the same JSON string-literal escaping and is emitted as `$env."NAME"`/`$env.<quoted-name>`. | ||
| - Command-backed values are intentionally executable Nushell expressions emitted as `(<command>)`. These commands come from Warp's environment-variable command model and are not escaped as data; callers must treat them as trusted command snippets. |
There was a problem hiding this comment.
try-block updater contract is underspecified. Define the exact Nushell syntax, minimum supported Nushell version, and how each package-manager sequence propagates non-zero external exits so warp_finish_update cannot run after a failed command.
There was a problem hiding this comment.
Fixed in 5c79bf4: the updater section now specifies the exact outer try/catch shape, the Nushell 0.109.x failure-propagation behavior it relies on, and how Apt, Yum, Dnf, Zypper, and Pacman sequences abort before warp_finish_update after a non-zero package-manager/setup command.
|
/oz-review |
Co-Authored-By: Warp <agent@warp.dev>
|
/oz-review |
There was a problem hiding this comment.
Overview
This spec PR defines product and technical requirements for first-class local Nushell support in Warp. The scope is generally well bounded around local shell detection, bootstrap, environment-variable syntax, update commands, and unsupported remote/subshell paths.
Concerns
- The technical spec does not fully specify how Nushell metadata promised by the product spec will be collected and tested beyond aliases/history locations.
- The compatibility/testing plan depends on Nushell 0.109.x and a local
nubinary, but does not state how that version is provisioned or pinned for CI/reviewer validation. - The local verification commands should be reframed as planned, repo-relative validation steps rather than author-local commands already used in this spec-only PR.
Verdict
Found: 0 critical, 2 important, 1 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
|
|
||
| 3. Add Nushell bootstrap assets. | ||
| - `nu_init_shell.nu` sends the initial `InitShell` hook and establishes `WARP_SESSION_ID`. | ||
| - `nu_body.nu` installs Nushell functions/hooks for `Preexec`, `Precmd`, `CommandFinished`, `Bootstrapped`, `Clear`, `InputBuffer`, `FinishUpdate`, prompt-mode toggles, PATH append, and initial working directory handling. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in e1b340a: added a dedicated Nushell metadata collection contract table for every Behavior 8 payload field, including source expressions, wire formats, and test expectations for shell version, PATH, history, aliases, custom commands, built-ins, keywords, env names, OS/WSL fields, and shell_path.
| - Special-case Linux package-manager update commands for `ShellType::Nu` so package-manager commands and `warp_finish_update` run inside one outer Nushell block with the exact shape `try { <package-manager sequence>; warp_finish_update <update_id> } catch { print $in }`. | ||
| - The contract depends on Nushell `0.109.x` behavior: a non-zero external command exit inside `try` raises an error, jumps to `catch`, and skips the remaining statements in that `try` block. This is the success gate that prevents `warp_finish_update` from running after a failed package-manager command. | ||
| - Apt sequence: optionally run `try { warp_handle_dist_upgrade <repo> } catch { null }` first. That inner failure is intentionally ignored to preserve the existing dist-upgrade best-effort behavior. Then run `sudo apt update; sudo apt install <package>`. Failure in either sudo command aborts the outer `try` before finish. | ||
| - Yum/Dnf/Zypper sequence: run the single package-manager upgrade/update command inside the outer `try`; any non-zero exit aborts before finish. |
There was a problem hiding this comment.
nu binary for smoke tests; define the provisioning/pinning or skip behavior so compatibility validation is reproducible.
There was a problem hiding this comment.
Fixed in e1b340a: added reproducible Nushell provisioning requirements: use a pinned Nix input or CI/devshell package, assert version >= 0.109.0 before smoke tests, and explicitly skip Nushell smoke/interactive tests when no binary is available instead of relying on an author-local profile.
|
|
||
| ## Testing and validation | ||
|
|
||
| Product behavior coverage: |
There was a problem hiding this comment.
💡 [SUGGESTION] This section describes commands as already used and then lists author-local /home/vitalyr/... paths even though the PR is spec-only; reframe them as planned implementation validation commands using repo-relative invocations.
There was a problem hiding this comment.
Fixed in e1b340a: reframed validation as planned repo-root implementation commands and replaced author-local absolute paths with direct cargo commands plus an optional relative devshell wrapper.
Co-Authored-By: Warp <agent@warp.dev>
|
Addressed the latest Oz feedback in e1b340a (metadata contract, reproducible Nushell provisioning, and repo-root validation commands). We have used the three /oz-review retries on this PR; @oss-maintainers could you please re-run review or advise next steps? |
There was a problem hiding this comment.
Overview
This spec PR adds product and technical specs for first-class local Nushell support, including shell detection, session spawning, bootstrap hooks, metadata collection, environment-variable serialization, Drive export behavior, update-command construction, and validation coverage.
Concerns
- The product spec claims cross-platform Nushell support, including Windows
nu.exe, but its history/config-file behavior only enumerates Unix-style locations and does not require platform-derived Nushell paths. - The technical spec includes WSL/MSYS2 spawning in the first iteration without matching validation coverage or a scoped fallback plan.
- The alias metadata contract uses tab/newline delimiters without defining escaping for delimiter-containing alias expansions.
Verdict
Found: 0 critical, 2 important, 1 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
|
|
||
| 8. Warp discovers Nushell metadata that is useful to existing terminal features: shell version, home directory, PATH, history file when Nushell uses plaintext history, aliases, custom commands, built-ins, keywords, environment-variable names, editor, OS category, Linux distribution, WSL distro name, and shell path. | ||
|
|
||
| 9. Warp reads the normal Nushell history and configuration locations when presenting shell metadata or file references. This includes `~/.config/nushell/history.txt`, `~/.local/share/nushell/history.txt`, `~/.config/nushell/env.nu`, `~/.config/nushell/config.nu`, and `~/.config/nushell/login.nu` where applicable. |
There was a problem hiding this comment.
nu.exe; require platform-derived $nu.config-path, $nu.env-path, $nu.loginshell-path, and $nu.history-path coverage so Windows and macOS local sessions are not omitted.
There was a problem hiding this comment.
Updated in 27820a4: product behavior 9 now requires Nushell runtime-derived paths instead of Unix-only defaults, and the tech spec includes config_path/env_path/loginshell_path/history-path fields sourced from the running Nushell instance.
| - For direct local sessions, invoke Nushell as `nu --login --execute <warp init script>`. The `--login` flag preserves normal login-shell startup, and `--execute` runs Warp's init snippet before leaving the process in an interactive shell. | ||
| - Startup ordering is part of the contract: Nushell loads the user's normal startup files first (`env.nu`, then `config.nu`, and `login.nu` for login shells), then Warp's `--execute` init script runs. Warp does not skip user config for the main interactive session. | ||
| - If a user startup file throws an error before `--execute` runs, Nushell reports that error and Warp treats the shell like any other shell that failed to bootstrap; Warp does not mask or rewrite user config failures. | ||
| - Support WSL and MSYS2 spawning when the detected shell basename maps to `ShellType::Nu`. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated in 27820a4: the WSL/MSYS2 scope now has explicit validation requirements for Nushell spawn arguments and unsupported-shell fallback behavior, rather than leaving those paths implied.
| | `home_dir` | `$nu.home-path?` with `$env.HOME?` fallback | string path | non-empty when Nushell exposes a home path | | ||
| | `path` | `$env.PATH` normalized by `warp_path_string` | platform path string joined with `(char esep)` when `$env.PATH` is a list; raw string otherwise | list and string PATH inputs both produce a path string | | ||
| | `editor` | `$env.EDITOR? | default ""` | string | reflects EDITOR or empty | | ||
| | `aliases` | `scope aliases | each {|alias| $"($alias.name)\t($alias.expansion? | default "")" } | str join (char nl)` | newline-separated rows; each row is tab-separated `name<TAB>expansion` | parser accepts tab rows and preserves expansion text | |
There was a problem hiding this comment.
💡 [SUGGESTION] The alias payload format uses tab/newline delimiters, but alias expansions can contain those characters; define escaping or switch to structured JSON rows before requiring the parser to preserve expansion text.
There was a problem hiding this comment.
Updated in 27820a4: switched the alias metadata contract from tab/newline-delimited rows to structured JSON objects so expansions containing tabs, newlines, quotes, or semicolons are preserved safely.
|
I rewrote the latest commit message to be more specific about the actual spec changes. The branch now points at 27820a4, That commit addresses the latest review pass by making these concrete changes:
I also did a quick local sanity check that the |
There was a problem hiding this comment.
Overview
This spec PR adds product and technical specs for first-class local Nushell support, covering shell detection, local spawning, bootstrap assets, metadata collection, environment-variable syntax, updater commands, and validation.
Concerns
- The tech spec contradicts itself on Nushell alias transport: one section says tab-separated parsing while the metadata contract requires JSON to preserve tabs and newlines.
- The bootstrap transport is ambiguous between
--executeand rc-file bootstrap, which could lead to an implementation that does not match the stated startup ordering. - Nushell-safe control-flow handling is only specified for Linux updater commands even though the product requirement covers Warp-generated success-gated flows more broadly.
Security
- The executable environment-variable command path names a trust boundary but should spell out how command snippets are constrained to trusted user-owned definitions so synced/imported text cannot become implicit shell code.
Verdict
Found: 0 critical, 3 important, 1 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
| - Parse markdown language specs `nu` and `nushell` as Nushell. | ||
| - Keep `ShellFamily::from(ShellType::Nu)` as `Posix` only for legacy family-level behavior, but avoid converting that family back to Bash in Nushell-specific export paths. | ||
| - Add Nushell history and rcfile locations. | ||
| - Parse Nushell aliases from tab-separated bootstrap output. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated in 9216a62 — the stale tab-separated alias plan is gone. The spec now makes JSON the only Nushell alias wire format, matching the metadata contract and preserving tabs/newlines/quotes/semicolons in alias expansions.
| - Bootstrap must merge with user configuration rather than replacing it: prepend Warp's `pre_execution` and `pre_prompt` hooks ahead of existing hook lists, append Warp keybindings to existing keybindings, and store the user's original prompt closures/strings before changing prompt indicators. | ||
| - Warp-managed prompt mode sets Nushell prompt indicators to empty strings and emits Warp prompt escape sequences. Honor-user-prompt mode restores the stored user prompt indicators and calls the stored `PROMPT_COMMAND`/`PROMPT_COMMAND_RIGHT` values when they are closures or strings. | ||
| - Disable Nushell's built-in OSC 133/633 shell integration flags in `$env.config.shell_integration` while Warp's integration is active to avoid duplicate prompt markers. | ||
| - Use rc-file bootstrap for local Nushell where Warp already uses that method for shells that should not receive large bootstrap payloads through the PTY. |
There was a problem hiding this comment.
nu --login --execute <warp init script> startup contract unless the init script only sources an rc-file. Specify whether local bootstrap is passed via --execute, written to an rc file and sourced, or split between the two, including ordering.
There was a problem hiding this comment.
Updated in 9216a62 — clarified this as a split bootstrap flow: user Nushell startup files run first, the early InitShell hook runs through nu --login --execute <nu_init_shell.nu> where supported, and the larger nu.nu/nu_body.nu bootstrap is sourced from a session-owned temp file afterward. The fallback path has to preserve that ordering instead of silently changing semantics.
| - Warp Drive export and copy paths carry `ShellType` from the active terminal session so Nushell sessions emit Nushell syntax. | ||
|
|
||
| 6. Add Nushell-safe update command construction. | ||
| - Keep generic `and_combiner()` behavior unchanged for existing shells. |
There was a problem hiding this comment.
and_combiner() POSIX. Enumerate all generated && call sites and define their Nushell behavior or explicitly mark them unsupported.
There was a problem hiding this comment.
Updated in 9216a62 — added a generated-command inventory. Today app/src/autoupdate/linux.rs is the only and_combiner() consumer, so the spec requires the Nushell updater branch to bypass POSIX && and test that no && is emitted. Future Nu-reachable generated-command call sites must add a Nu branch/test or route to unsupported-shell handling.
| 5. Add Nushell environment-variable serialization. | ||
| - Constants serialize as JSON string literals assigned to `$env`, using `serde_json::to_string`. This makes quotes, backslashes, newlines, `$()`, semicolons, and other shell-significant characters data rather than executable Nushell syntax. | ||
| - Bare environment names are allowed only when they match `[A-Za-z_][A-Za-z0-9_]*`. Any other name serializes through the same JSON string-literal escaping and is emitted as `$env."NAME"`/`$env.<quoted-name>`. | ||
| - Command-backed values are intentionally executable Nushell expressions emitted as `(<command>)`. These commands come from Warp's environment-variable command model and are not escaped as data; callers must treat them as trusted command snippets. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Executable command snippets are a shell-injection trust boundary; document how the implementation ensures these commands only originate from trusted user-owned environment-variable definitions and are not sourced from imported/shared text without an explicit user action.
There was a problem hiding this comment.
Updated in 9216a62 — documented the trust boundary for command-backed and secret-backed env vars. Drive sync, preview, import, shared collection browsing, and metadata rendering must keep snippets inert; execution only happens after explicit user intent in an owned or explicitly trusted collection, otherwise the path must render for review or stay unsupported.
Require the Nushell spec to use platform-derived $nu paths for history and config files instead of Unix-only defaults. Add explicit WSL/MSYS2 spawn and fallback validation so those scoped paths are testable. Switch alias metadata from delimiter-separated rows to JSON objects so alias expansions containing tabs, newlines, quotes, or semicolons round-trip. Co-Authored-By: Warp <agent@warp.dev>
VitalyAnkh
force-pushed
the
vitaly/issue-2038-nushell-spec
branch
from
8220791 to
27820a4
Compare
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for first-class local Nushell support in Warp, covering detection, spawning, bootstrap assets, metadata collection, environment-variable syntax, update commands, and validation.
Concerns
- The tech spec leaves Nushell in
ShellFamily::Posixwithout requiring an inventory/test contract for every remaining POSIX-family call site, which risks Bash/POSIX syntax leaking into Nushell paths despite the product spec's unsupported-shell requirement. - The alias parsing plan is internally inconsistent: one section says tab-separated bootstrap output, while the metadata contract requires JSON that preserves tabs and newlines in alias expansions.
Security
- The environment-variable command substitution section names command snippets as trusted but should document where that trust is enforced, especially for any Drive/shared/imported environment-variable content.
Verdict
Found: 0 critical, 2 important, 1 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
| 1. Add `ShellType::Nu` as a first-class shell type. | ||
| - Detect `nu`, `-nu`, Unix paths ending in `/nu`, and Windows basenames exactly equal to `nu.exe`. | ||
| - Parse markdown language specs `nu` and `nushell` as Nushell. | ||
| - Keep `ShellFamily::from(ShellType::Nu)` as `Posix` only for legacy family-level behavior, but avoid converting that family back to Bash in Nushell-specific export paths. |
There was a problem hiding this comment.
ShellFamily::Posix leaves generic POSIX call sites eligible; require an inventory/test contract for every remaining family-level path that is safe, gated as Nushell-specific, or routed to unsupported-shell handling.
There was a problem hiding this comment.
Updated in 9216a62 — added a ShellFamily::Posix inventory for Nu-reachable paths and classified each area as safe data escaping, Nu-gated syntax, or unsupported. The testing section now requires regression coverage or an explicit unsupported-shell assertion for each inventory entry before the implementation PR can land.
| - Parse markdown language specs `nu` and `nushell` as Nushell. | ||
| - Keep `ShellFamily::from(ShellType::Nu)` as `Posix` only for legacy family-level behavior, but avoid converting that family back to Bash in Nushell-specific export paths. | ||
| - Add Nushell history and rcfile locations. | ||
| - Parse Nushell aliases from tab-separated bootstrap output. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated in 9216a62 — JSON is now the only alias wire format for Nushell. The earlier tab-separated wording was removed so the proposed change and metadata contract agree.
| 5. Add Nushell environment-variable serialization. | ||
| - Constants serialize as JSON string literals assigned to `$env`, using `serde_json::to_string`. This makes quotes, backslashes, newlines, `$()`, semicolons, and other shell-significant characters data rather than executable Nushell syntax. | ||
| - Bare environment names are allowed only when they match `[A-Za-z_][A-Za-z0-9_]*`. Any other name serializes through the same JSON string-literal escaping and is emitted as `$env."NAME"`/`$env.<quoted-name>`. | ||
| - Command-backed values are intentionally executable Nushell expressions emitted as `(<command>)`. These commands come from Warp's environment-variable command model and are not escaped as data; callers must treat them as trusted command snippets. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] This says command snippets are trusted but not where that trust is enforced; state whether command-backed values can originate from Drive/shared/imported content and require the existing confirmation or owner-only boundary before executing them in Nushell.
There was a problem hiding this comment.
Updated in 9216a62 — the env-var section now states where trust is enforced: shared/imported Drive content stays inert during sync/preview/import/browsing, and command-backed values need explicit user action plus ownership or explicit trust before they can execute in a Nushell session.
Resolve spec review gaps around Nushell's remaining ShellFamily::Posix surface. The tech spec now inventories Nu-reachable family-level call sites, defines which are safe, gated, or unsupported, and requires regression coverage before new POSIX-family paths can emit shell syntax. Clarify the split bootstrap flow: user startup files run first, the early InitShell hook runs through --execute, and the larger body bootstrap is sourced from a session-owned temp file. Make alias metadata JSON-only and document the env-var command execution trust boundary for Drive/shared/imported content. Co-Authored-By: Warp <agent@warp.dev>
|
Pushed 9216a62 for the latest spec review pass. What changed:
Local checks I ran:
I’m not posting another |
There was a problem hiding this comment.
Overview
This spec PR adds product and technical specs for first-class local Nushell support, including shell detection, local spawning, bootstrap assets, metadata collection, environment-variable serialization, Linux updater command construction, validation, risks, and follow-ups.
Concerns
- The bootstrap delivery contract is ambiguous about the exact command string sent to
nu --executeand how the later temp-file bootstrap is sourced, which could lead to an implementation that does not load Warp's init/body scripts reliably. - The updater command contract needs to require Nushell-safe quoting for every interpolated value before constructing privileged package-manager command blocks.
Security
- Generated update commands need explicit escaping requirements for dynamic fragments used inside root/package-manager command blocks.
- Secret-backed environment-variable flows should document redaction boundaries for generated secret-manager commands and secret command output.
Verdict
Found: 0 critical, 2 important, 1 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
| 2. The early Warp init script is `nu_init_shell.nu`. Direct Unix/macOS sessions and WSL sessions pass it through `nu --login --execute <nu_init_shell.nu>`, so Nushell remains interactive after the init snippet runs. If a platform-specific launcher cannot carry `--execute` safely, the implementation must source a temp file whose first statement is the same init script before it sources the body script. | ||
| 3. The larger Warp body bootstrap is not embedded in the `--execute` argument. Warp writes the rendered `nu.nu`/`nu_body.nu` body to the same temp-file source bootstrap path used for shells that should not receive large bootstrap payloads directly through the PTY, then asks Nushell to `source` that file after the process is running. If temp-file bootstrap setup fails, Warp may fall back to the existing bracketed-paste/bootstrap writer, but it must preserve the same ordering. |
There was a problem hiding this comment.
--execute and the later source step. --execute evaluates a command string, and source/source-env need a correctly quoted literal path; a bare temp-file path or dynamically built source path can fail to load the bootstrap.
There was a problem hiding this comment.
Updated in 99695f7 — made the bootstrap handoff explicit. --execute receives the rendered nu_init_shell.nu body as one argv value, not a path or generated source command. The later body load is exactly source <temp_bootstrap_path_literal>\n, with the temp path rendered as a Nushell double-quoted/JSON-style literal, and the spec calls out why this must be source rather than source-env.
|
|
||
| 6. Add Nushell-safe update command construction. | ||
| - Keep generic `and_combiner()` behavior unchanged for existing shells. | ||
| - Special-case Linux package-manager update commands for `ShellType::Nu` so package-manager commands and `warp_finish_update` run inside one outer Nushell block with the exact shape `try { <package-manager sequence>; warp_finish_update <update_id> } catch { print $in }`. |
There was a problem hiding this comment.
<package>, <repo>, <update_id>, cache paths) before building this privileged command block; otherwise a malformed value can alter the generated command structure.
There was a problem hiding this comment.
Updated in 99695f7 — added the escaping contract for the privileged updater block. Dynamic package/repo/update-id/cache/path values must be bound from Nushell-safe string literals before command construction, passed as arguments, and covered by tests with quotes, semicolons, $(), newlines, spaces, and leading dashes.
| - Constants serialize as JSON string literals assigned to `$env`, using `serde_json::to_string`. This makes quotes, backslashes, newlines, `$()`, semicolons, and other shell-significant characters data rather than executable Nushell syntax. | ||
| - Bare environment names are allowed only when they match `[A-Za-z_][A-Za-z0-9_]*`. Any other name serializes through the same JSON string-literal escaping and is emitted as `$env."NAME"`/`$env.<quoted-name>`. | ||
| - Command-backed values are intentionally executable Nushell expressions emitted as `(<command>)`. These commands come from Warp's environment-variable command model and are not escaped as data; callers must treat them as trusted command snippets. | ||
| - Secret-backed values are intentionally executable command substitutions around the external secret-manager retrieval command. The secret reference/manager configuration is the trust boundary, and the command output becomes the environment value. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Add a redaction requirement for secret-backed values: generated secret-manager commands and their stdout should not be stored in Drive metadata, telemetry, debug logs, or bootstrap/apply failure output.
There was a problem hiding this comment.
Updated in 99695f7 — added the redaction boundary for secret-backed env vars. Drive may keep the structured secret reference, but rendered secret-manager commands, stdout, and potentially secret-bearing stderr must not be stored in Drive metadata, telemetry, debug logs, or apply/bootstrap failure output.
Clarify the exact Nushell startup contract for the spec: --execute receives the rendered init script as a single argv value, while the larger body bootstrap is loaded later with source plus a JSON-escaped temp path literal. Require privileged updater blocks to bind every dynamic package, repo, update-id, cache, and path value through Nushell-safe string literals before command construction. Add validation requirements for malicious-looking values and redaction boundaries for secret-backed environment-variable commands and output. Co-Authored-By: Warp <agent@warp.dev>
|
Pushed 99695f7 for the latest review pass. This update tightens three spots:
Local checks after the change:
The three automatic |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for first-class local Nushell support, covering shell detection, spawning, bootstrap assets, metadata collection, env-var serialization, Drive export behavior, updater commands, and validation.
Concerns
- The spec currently treats JSON string escaping as a valid Nushell string-literal contract, but Nushell uses different Unicode escape syntax; this would make generated bootstrap/env-var commands fail for some legal values.
- Direct Windows
nu.exeand MSYS2 sessions are in product scope, but the detailed bootstrap contract only specifies Unix/macOS and WSL delivery. - The metadata contract names
$nu.home-path?, which does not match Nushell's documented$nu.home-dirfield. - One security-relevant temp-file requirement should be explicit before implementation.
Security
- The session bootstrap temp file is executable code sourced by the user's shell, so the spec should require secure creation with owner-only permissions and no symlink/preexisting-file reuse.
Verdict
Found: 0 critical, 4 important, 1 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
|
|
||
| ## Behavior | ||
|
|
||
| 1. Warp recognizes Nushell executables named `nu`, login-shell aliases named `-nu`, Unix paths ending in `/nu`, and Windows executable basenames exactly equal to `nu.exe`. |
There was a problem hiding this comment.
💡 [SUGGESTION] Clarify Windows matching case-sensitivity: compare extracted Windows executable basenames case-insensitively while still rejecting unrelated names such as menu.exe.
There was a problem hiding this comment.
Updated in 661cb5b — product behavior now says Windows executable basenames are compared case-insensitively against the whole nu.exe filename, while still rejecting unrelated names like menu.exe and NuGet.exe.
| - `nu.nu` includes the body script through the existing bundled asset mechanism. | ||
| - Split startup/bootstrap delivery deliberately: | ||
| 1. User startup files run first as part of the login Nushell process. | ||
| 2. The early Warp init script is `nu_init_shell.nu`. Direct Unix/macOS sessions and WSL sessions pass the rendered script contents as the single argument to `--execute`: argv is `["nu", "--login", "--execute", <rendered nu_init_shell.nu contents>]` after replacing `nu` with the selected shell path. The rendered command string must be the script body itself, with build placeholders such as `@@USING_CON_PTY_BOOLEAN@@` substituted before process spawn; it is not a path and it does not dynamically construct a `source` command. Nushell remains interactive after this init snippet runs. |
There was a problem hiding this comment.
nu.exe and MSYS2 are in scope, but this bootstrap contract only names Unix/macOS and WSL; specify their exact argv and whether they use --execute or the temp-source fallback.
There was a problem hiding this comment.
Updated in 661cb5b — the bootstrap contract now covers direct Windows and MSYS2 explicitly. Direct Windows nu.exe uses the same --login --execute <rendered init body> argv shape as direct Unix/macOS; MSYS2 stays on the slow-PTY-safe temp-source fallback with --login, sourcing a temp file that contains init followed by body.
| - Split startup/bootstrap delivery deliberately: | ||
| 1. User startup files run first as part of the login Nushell process. | ||
| 2. The early Warp init script is `nu_init_shell.nu`. Direct Unix/macOS sessions and WSL sessions pass the rendered script contents as the single argument to `--execute`: argv is `["nu", "--login", "--execute", <rendered nu_init_shell.nu contents>]` after replacing `nu` with the selected shell path. The rendered command string must be the script body itself, with build placeholders such as `@@USING_CON_PTY_BOOLEAN@@` substituted before process spawn; it is not a path and it does not dynamically construct a `source` command. Nushell remains interactive after this init snippet runs. | ||
| 3. The larger Warp body bootstrap is not embedded in the `--execute` argument. Warp writes the rendered `nu.nu`/`nu_body.nu` body to the same temp-file source bootstrap path used for shells that should not receive large bootstrap payloads directly through the PTY, then sends exactly one source command after the process is running: `source <temp_bootstrap_path_literal>\n`. The path literal must be a Nushell double-quoted string literal produced by the same escaping contract as JSON strings (`serde_json::to_string` over the temp path is acceptable and must be tested), so spaces, quotes, backslashes, brackets, and other path characters cannot change the command structure. Use `source`, not `source-env`, because the body defines Nushell functions and updates `$env`/`$env.config`. |
There was a problem hiding this comment.
serde_json::to_string is not a general Nushell string-literal serializer because JSON emits \u001b/\u0000 escapes while Nushell expects \u{...}; define a Nushell-specific serializer and test control characters for temp paths and env values.
There was a problem hiding this comment.
Updated in 661cb5b — replaced the JSON-string shortcut with a Nushell-specific double-quoted literal serializer. The spec now requires \u{hex} for control characters, rejects JSON-only \u00xx escapes for generated Nu source, and applies the serializer to temp paths, env values/names, and updater dynamic values.
| 2. The early Warp init script is `nu_init_shell.nu`. Direct Unix/macOS sessions and WSL sessions pass the rendered script contents as the single argument to `--execute`: argv is `["nu", "--login", "--execute", <rendered nu_init_shell.nu contents>]` after replacing `nu` with the selected shell path. The rendered command string must be the script body itself, with build placeholders such as `@@USING_CON_PTY_BOOLEAN@@` substituted before process spawn; it is not a path and it does not dynamically construct a `source` command. Nushell remains interactive after this init snippet runs. | ||
| 3. The larger Warp body bootstrap is not embedded in the `--execute` argument. Warp writes the rendered `nu.nu`/`nu_body.nu` body to the same temp-file source bootstrap path used for shells that should not receive large bootstrap payloads directly through the PTY, then sends exactly one source command after the process is running: `source <temp_bootstrap_path_literal>\n`. The path literal must be a Nushell double-quoted string literal produced by the same escaping contract as JSON strings (`serde_json::to_string` over the temp path is acceptable and must be tested), so spaces, quotes, backslashes, brackets, and other path characters cannot change the command structure. Use `source`, not `source-env`, because the body defines Nushell functions and updates `$env`/`$env.config`. | ||
| 4. If a platform-specific launcher cannot carry `--execute` safely, the implementation must write a temp file whose first rendered statements are the `nu_init_shell.nu` contents followed by the `nu.nu`/`nu_body.nu` contents, then source that file with the same `source <temp_bootstrap_path_literal>` command. If temp-file bootstrap setup fails, Warp may fall back to the existing bracketed-paste/bootstrap writer, but it must preserve the same ordering and literal-path escaping. | ||
| - Do not modify the user's `env.nu`, `config.nu`, or `login.nu`; the temp source file is owned by the Warp session and cleaned up like other RC-file bootstrap artifacts. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated in 661cb5b — added secure temp bootstrap file requirements: exclusive creation, no symlink or preexisting-file reuse, owner-only permissions/ACLs before sourcing, and no cross-session path reuse.
| - `config_path`: source is `$nu.config-path? | default ""`; sent as a string path equal to Nushell's platform-derived `config.nu` path when available. | ||
| - `env_path`: source is `$nu.env-path? | default ""`; sent as a string path equal to Nushell's platform-derived `env.nu` path when available. | ||
| - `loginshell_path`: source is `$nu.loginshell-path? | default ""`; sent as a string path equal to Nushell's platform-derived `login.nu` path when available. | ||
| - `home_dir`: source is `$nu.home-path?` with `$env.HOME?` fallback; sent as a non-empty string path when Nushell exposes a home path. |
There was a problem hiding this comment.
$nu.home-dir, not $nu.home-path; update this source field or the bootstrap metadata test will assert the wrong value.
There was a problem hiding this comment.
Updated in 661cb5b — I checked local Nushell 0.109.1 and it exposes $nu.home-path while $nu.home-dir is absent, so the spec now probes both runtime field names with an env fallback. That keeps tests tied to the running Nu version instead of assuming one spelling.
Replace the previous JSON-string shortcut for generated Nushell source with an explicit Nu double-quoted literal serializer that handles quotes, backslashes, control characters, and Unicode escapes. Apply that contract to temp bootstrap paths, env vars, and updater dynamic values. Clarify direct Windows nu.exe argv, MSYS2 temp-source fallback, secure temp bootstrap file creation, and case-insensitive Windows basename matching. Make home-directory metadata probe both Nushell runtime field names so tests follow the running Nu version instead of assuming one spelling. Co-Authored-By: Warp <agent@warp.dev>
|
Pushed 661cb5b for the latest review pass. What changed:
Local checks after the change:
Automatic |
There was a problem hiding this comment.
Overview
This spec PR defines product and technical requirements for first-class local Nushell support, including detection, spawning, bootstrap assets, metadata collection, environment-variable serialization, Drive export behavior, and Linux update command construction.
Concerns
- The startup-order contract omits Nushell vendor/user autoload directories, which are part of normal user startup configuration and should be included in the ordering and validation coverage.
- The privileged updater command contract treats leading-dash dynamic values as safe once escaped into one argument, but external commands can still interpret those values as options unless the spec requires option terminators or command-specific validation.
Security
- The Nushell updater design should explicitly prevent option injection for dynamic arguments passed to sudo/package-manager/file commands.
Verdict
Found: 0 critical, 2 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
| - Include `nu`/`nu.exe` in shell discovery and display it as "Nushell". | ||
| - Add `/bin/nu` fallback on Unix-like systems. | ||
| - For direct local sessions, invoke Nushell as `nu --login --execute <warp init script>`. The `--login` flag preserves normal login-shell startup, and `--execute` runs Warp's init snippet before leaving the process in an interactive shell. | ||
| - Startup ordering is part of the contract: Nushell loads the user's normal startup files first (`env.nu`, then `config.nu`, and `login.nu` for login shells), then Warp's `--execute` init script runs. Warp does not skip user config for the main interactive session. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated in 0c43e63 — the startup-order contract now includes vendor and user autoload directories. The validation plan uses marker scripts for env.nu, config.nu, login.nu, vendor autoload, and user autoload and asserts all markers run before Warp init/bootstrap markers.
| - Special-case Linux package-manager update commands for `ShellType::Nu` so package-manager commands and `warp_finish_update` run inside one outer Nushell block with the exact shape `try { <package-manager sequence>; warp_finish_update <update_id> } catch { print $in }`. | ||
| - Every dynamic fragment interpolated into the Nushell updater block must be rendered before command construction as a Nushell string literal or bound local variable using the Nushell literal serializer above. This includes package names, repository names, update ids, cache/temp paths, backup paths, and generated config paths. Do not concatenate raw dynamic strings into external command text or into the `warp_finish_update` call. | ||
| - The preferred Nushell shape is to bind dynamic values once, for example `let warp_update_id = <escaped string literal>` and `let warp_package_name = <escaped string literal>`, then pass those variables as arguments to external commands and `warp_finish_update`. Package-manager selection and subcommand names are enum-controlled by Warp; only data arguments may come from dynamic values. | ||
| - Tests must include malicious-looking dynamic values containing spaces, quotes, semicolons, `$()`, newlines, and leading dashes, and assert those values remain a single Nushell string/argument without introducing extra statements or moving `warp_finish_update` outside the success gate. |
There was a problem hiding this comment.
-- where supported or command-specific validation, and assert the tests cover option-injection prevention.
There was a problem hiding this comment.
Updated in 0c43e63 — tightened this beyond shell escaping. Dynamic values passed to sudo, package managers, or file commands now need -- where the command supports it, or command-specific validation/rejection for leading-dash values before the block is constructed; tests must cover option-injection prevention.
Add Nushell vendor and user autoload directories to the startup-order contract so modular user config must run before Warp bootstrap, with validation markers for env, config, login, vendor autoload, and user autoload files. Strengthen the updater security contract beyond shell escaping by requiring option terminators where supported or command-specific validation/rejection for leading-dash dynamic values before invoking sudo, package-manager, or file commands. Co-Authored-By: Warp <agent@warp.dev>
|
Pushed 0c43e63 for the latest review pass. Changes:
Local checks:
Automatic |
There was a problem hiding this comment.
Overview
This spec PR adds product and technical specs for first-class local Nushell support, including shell detection, bootstrap delivery, metadata collection, environment-variable serialization, Drive export behavior, updater command construction, and validation coverage.
Concerns
- The tech spec codifies a Nushell startup order that appears to conflict with Nushell's documented config loading order, which would make implementation/tests assert behavior Warp should not control.
- The exact Nushell updater catch shape is underspecified/likely incorrect because it prints
$inwithout binding the caught error, leaving failure reporting ambiguous in the command contract.
Verdict
Found: 0 critical, 2 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
| - Include `nu`/`nu.exe` in shell discovery and display it as "Nushell". | ||
| - Add `/bin/nu` fallback on Unix-like systems. | ||
| - For direct local sessions, invoke Nushell as `nu --login --execute <warp init script>`. The `--login` flag preserves normal login-shell startup, and `--execute` runs Warp's init snippet before leaving the process in an interactive shell. | ||
| - Startup ordering is part of the contract: Nushell completes its normal user startup before Warp's init script runs. For the supported `0.109.x` line, that startup order is `env.nu`, `config.nu`, `login.nu` for login shells, `*.nu` files from `$nu.vendor-autoload-dirs`, then `*.nu` files from `$nu.user-autoload-dirs`. Autoload directories are processed in the order listed by the corresponding `$nu.*-autoload-dirs` field, and files within each directory are processed alphabetically. Warp does not skip user config or modular autoload config for the main interactive session. |
There was a problem hiding this comment.
login.nu before vendor/user autoload files, while Nushell documents login config after autoload; update the order or avoid asserting an exact order Warp does not control.
There was a problem hiding this comment.
Updated in 25c8c13 — removed the exact relative startup-order assertion. The spec now only requires Warp to let Nushell finish its normal env/config/login plus vendor/user autoload startup before Warp init/bootstrap, and the validation checks those markers happen before Warp without making Warp responsible for Nushell's internal order.
|
|
||
| 6. Add Nushell-safe update command construction. | ||
| - Keep generic `and_combiner()` behavior unchanged for existing shells. | ||
| - Special-case Linux package-manager update commands for `ShellType::Nu` so package-manager commands and `warp_finish_update` run inside one outer Nushell block with the exact shape `try { <package-manager sequence>; warp_finish_update <update_id> } catch { print $in }`. |
There was a problem hiding this comment.
catch { print $in } shape does not bind the caught error, so the spec should define a valid failure-reporting form such as catch {|err| print $err.rendered } before implementation codifies this contract.
There was a problem hiding this comment.
Updated in 25c8c13 — changed the updater catch contract to bind the error explicitly: catch {|err| print $err.rendered }. I also smoke-checked that shape locally with a failing external command under Nu.
Keep the startup contract focused on what Warp controls: Nushell must finish its normal env/config/login and vendor/user autoload startup before Warp bootstrap, but Warp should not replay or assert Nushell's internal relative order. Fix the updater catch contract by binding the caught Nushell error and printing its rendered message instead of relying on `$in`, so the specified command shape is valid and testable. Co-Authored-By: Warp <agent@warp.dev>
|
Pushed 25c8c13 for the latest review pass. Changes:
Local checks:
|
captainsafia
added
the
external-contributor
Warp Dev Github Integration
2 participants
Description
Adds the product and technical specs for Nushell support requested in #2038. The issue is currently labeled
ready-to-spec, so this PR documents the agreed first iteration before the implementation PR proceeds.Refs #2038
Testing
Docs/spec-only change. I verified the markdown files are present under
specs/GH2038/and rancargo fmt --all -- --checkafter adding them; no code changes are included in this spec branch.Server API dependencies
No server API dependencies.
Agent Mode
Changelog Entries for Stable
No changelog entry; spec-only PR.
Co-Authored-By: Warp agent@warp.dev