Skip to content

feat: extract bundled Nushell plugins by default in setup nu - #133

Open
kiro-agent[bot] wants to merge 5 commits into
masterfrom
feat/bundled-plugin-extraction
Open

feat: extract bundled Nushell plugins by default in setup nu#133
kiro-agent[bot] wants to merge 5 commits into
masterfrom
feat/bundled-plugin-extraction

Conversation

@kiro-agent

@kiro-agent kiro-agent Bot commented Aug 21, 2026

Copy link
Copy Markdown

Summary

Changes numan setup nu to extract the full official Nushell release archive (nu + all bundled plugins like polars, query, formats, gstat, inc) instead of filtering to just the nu binary.

Background

PR #101 added an include filter to skip bundled plugins because the 279 MiB uncompressed archive tripped the old 256 MiB bomb cap. The cap was raised to 512 MiB (sufficient), but the include filter remained — leaving a UX gap where users expect the same plugins they'd get from a manual Nushell install.

Changes

  • Default full extraction: numan setup nu now extracts everything into tools/nushell/<version>/
  • --minimal flag: Escape hatch that restores the old nu-binary-only behavior for users who want lean installs
  • Auto-discovery: After extraction, discover_bundled_plugins() scans for nu_plugin_* binaries, computes SHA256, and writes lockfile entries with origin: "bundled:nu"
  • List display: numan list shows (bundled with Nu) tag for these entries
  • Activate works: Bundled plugins use standard payload_path + executable_path resolution — no changes needed to activate

Collision safety

If a plugin already exists in the lockfile from a registry install (non-bundled origin), the discovery pass skips it rather than overwriting.

Files changed

  • src/state/lockfile.rsBUNDLED_NU_ORIGIN constant
  • src/cmd/setup.rs--minimal flag threading
  • src/nu/bootstrap.rs — conditional extract, copy_extracted_files, discover_bundled_plugins
  • src/cmd/list.rs — display tag
  • src/cmd/nu_pin_offer.rs — constructor update
  • tests/setup_nu_test.rs — 5 new integration tests

Verification

  • All 908 tests pass (766 lib + 142 integration)
  • cargo clippy -- -D warnings clean
  • cargo fmt --check clean

Non-blocking follow-ups

  • Streaming SHA256 for large plugin binaries (polars ~100MB) to reduce peak memory
  • Failure ordering between write_active_version and discover_bundled_plugins

Change default behavior of `numan setup nu` to extract the full
release archive (nu + all bundled plugins) into tools/nushell/<version>/.
The 512 MiB cap is already sufficient for the ~279 MiB archives.

Add `--minimal` flag to restore the old behavior (extract only the
nu binary, skip plugins).

After extraction in full mode, scan the version directory for
nu_plugin_* binaries and write lockfile entries with origin
"bundled:nu". These plugins become discoverable and activatable
via `numan activate` without requiring a registry install flow.

Update `numan list` to display "(bundled with Nu)" tag for entries
with origin "bundled:nu", matching the existing nupm import tag
pattern.

Key changes:
- src/state/lockfile.rs: add BUNDLED_NU_ORIGIN constant
- src/cmd/setup.rs: add --minimal flag to NuSetupArgs
- src/nu/bootstrap.rs: conditional extract config, copy all extracted
  files, discover_bundled_plugins function, thread minimal through
  install chain
- src/cmd/list.rs: bundled origin tag display
- tests: unit and integration coverage for both modes
…om version dir

Address review feedback for bundled plugin extraction:

1. discover_bundled_plugins now skips entries that already exist with a
   non-bundled origin, preserving user registry installs over automatic
   bundled extraction. Existing bundled entries are still updated.

2. copy_extracted_files now only copies files matching the nu_plugin_*
   prefix, keeping the version directory clean (no README, LICENSE, etc.).

3. Added tests: collision with registry entry is skipped, collision with
   existing bundled entry is updated, and non-plugin archive files are
   excluded from the version directory.
@github-actions

Copy link
Copy Markdown
Contributor

User kiro-agent[bot] does not have write permissions

github run

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

User kiro-agent[bot] does not have write permissions

github run

@tonythethompson
tonythethompson self-requested a review August 22, 2026 10:38
@tonythethompson

Copy link
Copy Markdown
Collaborator

/oc review

@github-actions

Copy link
Copy Markdown
Contributor

I've completed a thorough review of PR #133. Here is my analysis.

Review: PR #133 — Extract bundled Nushell plugins by default in setup nu

Verification

  • cargo test --lib — 766 passed, 0 failed
  • cargo test --test setup_nu_test — 21 passed, 0 failed (3 ignored)
  • cargo clippy -- -D warnings — clean
  • cargo fmt --check — clean

The feature itself is well-implemented and well-tested: the --minimal flag threading, full-vs-minimal extraction, nu_plugin_*-only copy filter, lockfile discovery with collision-skip semantics, and the (bundled with Nu) list tag all work as described. The test coverage is solid (collision-skip, overwrite-bundled, non-plugin filtering, full-setup-flow lockfile write).

🔴 Blocking — numan remove <bundled-plugin> destroys the entire Nu installation

This is a data-loss bug that should block merge.

Bundled plugin entries set payload_path = "tools/nushell/<version>" — the shared Nu version directory, not a per-plugin directory. But numan remove assumes exclusive ownership of payload_path and calls remove_dir_all on it:

src/cmd/remove.rs:101-167

let payload_path = entry.payload_path().to_string();
let payload_dir = root.join(&payload_path);
...
if payload_dir.exists() {
    std::fs::remove_dir_all(&payload_dir) ...
}

So numan deactivate nushell/polars && numan remove nushell/polars would remove_dir_all(root/tools/nushell/0.114.0) — deleting the nu binary itself and every other bundled plugin in that version. There is no BUNDLED_NU_ORIGIN guard in remove.rs (only origin: None appears in a test fixture).

numan gc is safe — it only scans root/packages/ for orphan candidates (gc.rs:63-72), so it won't touch tools/nushell/. numan update is likely safe (no registry entry to resolve a new version from), but remove is the clear hazard.

Recommended fix: detect origin == BUNDLED_NU_ORIGIN in remove and either (a) refuse with a hint ("bundled plugins are removed with numan setup nu remove"), or (b) delete only the specific executable_path file rather than remove_dir_all on the shared directory. Option (a) is safer and avoids surprising the lockfile/activation-profile bookkeeping for the Nu install.

A test should cover: numan remove nushell/polars on a bundled entry must not delete tools/nushell/<version>/nu.

🟡 Non-blocking observations

  1. Short-circuit path skips discovery (bootstrap.rs ~line 1115). The "already installed" branch (options.yes && effective.is_file()) writes the active marker but never calls discover_bundled_plugins. A user who first ran setup nu --minimal then later runs full setup nu on the same version gets neither extraction nor discovery (short-circuit returns early). Consistent (plugins aren't on disk), but there's no minimal→full migration path on an already-installed version. Worth a documented limitation or a "re-extract" force path.

  2. Discovery-after-active-marker ordering (acknowledged in the PR body). discover_bundled_plugins runs after write_active_version; a discovery failure leaves the active marker set but no plugin entries, and re-running setup nu short-circuits (no re-discovery). There's no recovery path — consider ordering discovery before the active marker write, or adding a doctor/init --refresh reconciliation hook.

  3. doctor reconciliation gap. If discovery fails partway, numan doctor won't detect orphaned nu_plugin_* binaries on disk vs missing lockfile entries. Out of scope here, but worth a follow-up issue so the bundled set is self-healing like the other journals.

  4. In-memory SHA256 for large plugins (polars ~100MB). Acknowledged as a follow-up; fine for now.

Minor nits

  • copy_extracted_files derives extract_subdir = source.parent().unwrap_or(&extract_root)unwrap_or silently falls back to scanning extract_root if the binary is at the archive root, which could pick up stray nu_plugin_* files from a differently-shaped archive. Real Nu releases always nest under nu-<version>-<platform>/, so this is theoretical, but a log/warn on the fallback would aid debugging.
  • The new list_shows_bundled_with_nu_tag test spawns the real numan binary via CARGO_BIN_EXE_numan; it passes but is heavier than the in-process tests. Acceptable for an integration assertion.

Verdict

The core feature is clean and well-tested, but the shared-payload_path design collides with numan remove's remove_dir_all assumption. I'd ask for the remove-path guard + test before merging.

New%20session%20-%202026-08-22T10%3A39%3A41.144Z
opencode session  |  github run

@tonythethompson

Copy link
Copy Markdown
Collaborator

/oc fix

@github-actions

Copy link
Copy Markdown
Contributor

fatal: could not read Username for 'https://github.com': No such device or address

opencode session  |  github run

Co-authored-by: tonythethompson <tonythethompson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants