refactor(setup): route bootstrap non-TTY guard through require_tty_or_yes - #70
Conversation
…nfirm utility Breaking change: --remove, --use-path, --use-existing are replaced by subcommands (remove, path, use <path>). Hidden backward-compat flags emit deprecation warnings (remove in v0.3.0). - Add src/util/confirm.rs: confirm_or_auto / confirm_or_bail (non-TTY auto-confirms) - Replace 16 inline TTY-check sites across 8 files - Normalize all --yes help text to 'Skip confirmation prompts' - Redesign NuSetupArgs with Option<NuAction> subcommand + positional VERSION - Add NuSetupArgs constructors for internal callers (doctor, nu_pin_offer) - Add 8 CLI-parse tests + update all affected test assertions - Update README, CHANGELOG, docs/numan-doctor.md, AGENTS.md
- Prevent PATH subcommand from deleting active managed Nu - Guard loader overwrites with ownership verification - Reject incompatible legacy Nu setup flags
Fix remaining --version references in resolve.rs, tighten doctor_test assertion, add skip_path guard to legacy compat path, add negative tests for version+subcommand and legacy use_existing+skip_path.
Change 'your PATH Nu is not touched' to 'your existing Nu is not replaced' since setup nu does modify PATH by default.
Introduce a new `numan use <version>` command path (`cli`, `main`, and `cmd::use_cmd`) as a post-1.0 placeholder that currently fails with a clear guidance message to use `numan setup nu <version>`. Update roadmap/docs to reflect the consolidated plan filename, add post-1.0 side-by-side Nu management notes, and register the new command module in AGENTS metadata.
Fixed 4 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
- Add src/nu/version_manager.rs for active version tracking - Active version stored in <root>/nu_state/active-version.json - Installed versions discovered from <root>/tools/nushell/<version>/ - Helpers: read/write active version, list installed, check if installed - Migration logic for legacy single-binary installs - Update bootstrap.rs to install to versioned subdirectories - install_from_archive() now writes to <root>/tools/nushell/<version>/ - managed_nu_binary() delegates to version_manager for active version - Automatically sets newly installed version as active - Implement numan use command - numan use <version> — switch to specific installed version - numan use latest — switch to newest installed version - numan use list — show all installed versions with active marker - Validates version is installed before switching - Provides helpful hints when version not found - Auto-migrates legacy single-binary installs on first run - Remove Commands::Use from root init exclusion (now needs root) All 425 tests pass, clippy clean, fmt applied.
- Validate and normalize Nu versions - Propagate legacy migration errors - Handle invalid active markers gracefully - Avoid parent path panic - Update Nu setup guidance
…ompilation errors
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…master for setup.rs, snapshot.rs, cli.rs, bootstrap.rs; keep full use_cmd.rs implementation
wip: puts numan use + confirm-gate UX + migration journal + reconcile into a single baseline commit so the pr-migrate-legacy-installs split can branch off a known starting point. This commit does not represent either final PR; the next step surgically extracts migration into its own branch and reverts the migration hunks on feature/numan-use.
- Drop state/migration-journal.rs (moves to pr-migrate-legacy-installs). - Drop journal.migration_pending finding + reconcile block from cmd/doctor.rs. - Drop CMD_USE constant from util/hints.rs (fix-hint for the migration journal finding; the constant is migration-specific). - Drop pub mod migration_journal; from state/mod.rs. - Drop migrate_legacy_install call from cmd/use_cmd.rs (will be re-added on the migration branch once it ships -- use_cmd.rs keeps the snapshot + active-marker write). - Drop the migration_journal architectural bullet from AGENTS.md; keep the active-marker bullet (numan-use specific). - Pull version_manager.rs from pr-migrate-legacy-installs (already numan-use only). - The roadmap never had migration wording. Result: feature/numan-use is pure numan-use feature (PR 67). Migration pieces live on pr-migrate-legacy-installs. Co-authored-by: Codebuff <noreply@codebuff.com>
…e-existing)
- Add validate_version_for_path in version_manager.rs: explicit
entry point that rejects traversal / separators / non-semver so
install paths can opt in to validating a caller-supplied version
before joining it into the managed tree (CodeRabbit Critical
finding: 'Validate the version string before you join it into a path').
Keeps version_install_dir / version_binary returning PathBuf for
call-site ergonomics; callers handling untrusted input call
validate_version_for_path (or normalize_version) first.
- setup.rs hard-coded the removed '--use-existing' flag in a println
message. Drop the flag reference; the message now describes the
replacement ('replaced by user-supplied Nu') and matches the prompt
Greptile suggested.
CI: build + fmt + clippy -- -D warnings clean; 442 tests pass.
The unresolved PR 67 review finding (PRRT_kwDOTGvfHs6VpJrb) asks install_from_archive to place binaries under <root>/tools/nushell/<version>/nu rather than the legacy single-binary <root>/tools/nushell/nu. That keeps install a pure payload write and keeps active-marker persistence owned by execute_nu_setup_with_installer, matching AGENTS.md's 'only activate/deactivate modify Nu integration state' invariant. The companion unit test now asserts against version_manager::version_binary(root, "0.0.0-test") so a regression to the flat layout would fail loudly instead of silently clobbering every installed version on the next install. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
PRRT_kwDOTGvfHs6VpJrf (CLAUDE.md)
Drop the mandatory model-specific Co-Authored-By trailer; require
explicit user request before adding any attribution. HEREDOC guidance
preserved.
PRRT_kwDOTGvfHs6VpJrm (docs)
README.md: add numan use <version>/latest/list rows + common-flags row.
AGENTS.md: replace 'reserved stub' with current implemented behavior.
CHANGELOG.md: Unreleased Added section now covers numan use, versioned
install layout, strict version-path validation.
PRRT_kwDOTGvfHs6VpJrq (test seam)
src/util/confirm.rs gains confirm_or_auto_with_tty / require_tty_or_yes_with_tty
variants that let tests assert both branches unconditionally. activate.rs,
deactivate.rs and tests/activate_test.rs drop their is_terminal guards and
use the new variants directly. Three new unit tests cover the non-TTY bail,
non-TTY auto-confirm, and --yes-polarity paths.
PRRT_kwDOTGvfHs6VpJrv (doctor lock)
setup::execute_nu_repair acquires the mutation lock before delegating to
execute_nu_impl; doctor.rs both repair branches route through it. AGENTS.md
'Every mutating code path must acquire the mutation lock' invariant restored.
PRRT_kwDOTGvfHs6VpJrx (dedup)
The duplicated --skip-path reject message (legacy flag path + subcommand
path) collapses into reject_skip_path_for_off_path_registration so the two
branches can't drift apart.
PRRT_kwDOTGvfHs6VpJsA (loader marker)
Verified that assets/nushell-loader/loader.nu's first two lines already
match OWNERSHIP_MARKER ('# Generated and managed by Numan. Do not edit.'
+ '# Numan autoload schema: 1'). Existing setup.rs install flow's
VENDOR_LOADER write propagates the marker; assert_managed_file_owned will
no longer trip on overwrite. No code change required.
PRRT_kwDOTGvfHs6VpJsF (try help)
TryArgs.yes doc comment now explicitly calls out the managed-Nu pin
refusal boundary: --yes does NOT consent to silent managed-Nu install
or version switch.
PRRT_kwDOTGvfHs6VpJsS (resolve doc)
append_nu_mismatch_remediation doc comment corrected: PATH-touched-ness
is removed (setup nu may persist PATH unless --skip-path); messages
actually say 'your existing Nu is not replaced' and end with the
per-Nu-activation note.
Validation: cargo build / cargo fmt --check / cargo clippy -- -D warnings /
cargo test --lib (445 passed; 0 failed) / cargo test --test activate_test
(18 passed; 0 failed) on Windows toolchain.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
…up_nu_test refactor Addresses final 4 newly-surfaced PR 67 review threads: PRRT_kwDOTGvfHs6VpJss (Critical) Add an explicit ConfirmPolicy enum (AutoConfirm vs RefuseOnNonTTY) and a unified confirm_or_auto_with helper. Callers needing the refusing variant (destructive ops) pass it explicitly; idempotent setup callers keep auto-confirm-on-non-TTY. Default still auto- confirms, matching CI/script idempotency expectations. PRRT_kwDOTGvfHs6VpJst confirm.rs mod tests now exercises non-TTY bail, AutoConfirm policy pass-through, and yes-polarity short-circuit. Three new tests cover the branches that previously only ran via is_terminal-dependent paths. PRRT_kwDOTGvfHs6VpJsu hints.rs gains shell_quote(s) and uses it inside setup_nu_use_existing. Fix hints pointing at paths with spaces or shell metacharacters now copy-paste into a POSIX-ish shell. PRRT_kwDOTGvfHs6VpJsv Add NuSetupArgs::use_existing_for_test(path, skip_path, yes) and use it in tests/setup_nu_test.rs. Hand-listing eight struct fields silently drifts when fields are added. The constructor name (_for_test) signals intent; it is unconditional because integration tests build the lib without cfg(test). 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Adds scripts/check-roadmap-drift.py to detect the same kind of drift PR
67 hit: shipped-feature bullets whose wording still calls them 'stub',
'reserved', or 'post-1.0 reserved'. The check passes when the consolidated
roadmap is consistent with what code actually ships, and exits 1 otherwise.
Rules:
* The consolidated roadmap must declare the repo-local roadmap rule and
name numan-plugins/docs/roadmap.md and numan-registry/docs/roadmap.md
so future maintainers in either sibling repo know where cross-repo
truth lives.
* No bullet outside a deferral/post-1.0/explicitly-deferred heading may
contain 'stub', 'reserved', 'post-1.0 reserved', 'exists as a stub',
or 'is a reserved'. Bullets are 'shipped-feel' when they carry an
[x] checkbox, a 'Wave N closed' / 'Phase N (shipped)' marker, or a
`numan <verb>` / `scripts/<name>.py` command pattern.
* If docs/roadmap.md exists in a repo, it must link back to the
consolidated roadmap; missing local roadmap becomes a warning (so the
source-of-truth repo doesn't fail), not an error.
A negative test (inject the PR67 'exists as a stub in 0.1.x / post-1.0
reserved' bullet) fails the check with four matches; the pristine
roadmap succeeds.
Mirror this script + the same 'roadmap-drift' job into numan-plugins
and numan-registry with CONSOLIDATED_ROADMAP=/LOCAL_ROADMAP= env overrides
pointing at each repo's local roadmap.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
Catches the next stale roadmap claim before it misleads downstream
planning in numan-plugins or numan-registry. Each bullet in the
"Side-by-side Nu version management (numan use)" section is now either
backed by src/ code with a file:line pointer or visibly tagged as
"Vision only — not yet shipped."
Specific corrections (cross-repo claim → shipped behavior):
* "numan use list ... + per-version plugin counts" → drop the
inaccurate half; execute_list (src/cmd/use_cmd.rs) currently prints
version + (active) only. Counts are forward-looking.
* "PATH/shim: Numan does not manage a shim." → CONTRADICTION.
persist_user_path_unix (src/nu/bootstrap.rs:561) creates a
~/.local/bin/nu symlink via std::os::unix::fs::symlink on Unix;
on Windows it appends the binary's parent to the user PATH. The
bullet is rewritten into three accurate bullets: PATH (Unix) /
PATH (process-only) / Active marker ownership, each citing the
concrete call site.
* "Lockfile plugin_activation becomes keyed by Nu version" → partial.
Each PluginActivation already carries nu_version: String
(src/state/lockfile.rs:44); the "Switching activates/deactivates
automatically" companion claim is aspirational. Split into one
SHIPPED bullet and three Vision-only aspirational bullets so the
shipped structure stays separated from the future behavior.
* All three "Numan-level aliases (optional)" bullets → Vision only.
No numan alias command exists in src/cli.rs; tag each.
* Catalog implication + "Use this to drive backfill waves once
numan use ships" → Vision-tag the forward-looking clauses; keep
the verifiable cross-repo fact about numan-plugins/docs/backlog.json
schema v1 (verified outside this repo).
The check-roadmap-drift.py script still passes on the post-audit
roadmap (0 errors, 1 warning for the absent repo-local roadmap,
expected for numan). A re-run of the negative PR67 contradiction
injection still exits 1 with four forbidden-phrase matches, so the
guardrail survives the audit.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
Closes the third leg of the cross-repo compliance rail: PR 67's roadmap
drift shipped in numan/scripts, but a guard only one side knows about
isn't a guardrail. This commit lays down drop-in artifacts the sibling
repos can paste-and-PR so the drift check fires on both sides too.
Artifacts (under cross-repo-mirror/):
* numan-plugins/docs/roadmap.md — thin pointer back to consolidated
* numan-plugins/.github/workflows/roadmap-drift.yml — CI job, fetches
the consolidated roadmap from numan@master and runs
scripts/check-roadmap-drift.py against it; same shape for registry.
* numan-registry/docs/roadmap.md — same shape, registry-side.
* numan-registry/.github/workflows/roadmap-drift.yml
* snapshot-tests/mirror_dry_run.sh — local sanity check that
both sibling CI jobs would pass against the current state of the
script and the consolidated roadmap. ALL MIRRORS PASS (= 0 errors,
0 warnings) for both numan-plugins and numan-registry.
* README.md — explains the mirror contract
and the install instructions for the maintainer of the sibling
repos.
Mechanics:
* The CI workflows curl the consolidated roadmap straight from
numan@master each run — the sibling repo never holds a stale copy.
The fetch step greps for the sibling's repo-local roadmap path; if
the canonical doc ever forgets to mention a sibling, that sibling's
drift job fails loudly instead of degrading silently.
* The script is pulled from numan@master the same way. Both pinned
via standard GitHub raw URL with --fail-with-body so a downgrade
triggers a visible error rather than a soft pass.
* Defense in depth: a second grep step repeats the repo-local cross-
link check directly in case the script's logic ever regresses.
Script polish:
* scripts/check-roadmap-drift.py honours --help / -h before treating
argv[1] as a path; prints the module docstring and exits 0. CI
invocations and human invocations both stop crashing on stray --help.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNushell bootstrap now exposes a shared hoisted-consent audit formatter and requires an interactive terminal or explicit ChangesNushell setup consent
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
PR Summary by QodoRefactor setup bootstrap non-TTY guard through require_tty_or_yes
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
hoisted_audit_messageis meant to be reused by multiple destructive flows, consider moving it out ofnu::bootstrapinto a shared audit/confirm module so future callers don’t have to depend on bootstrap-specific code. - Relying on exact
(audit)strings for automation grep is quite brittle; you might want to introduce a small structured audit/logging helper (e.g., fixed tokens or fields around the prose) so the machine-readable contract is stable even if the human-facing text is later copy-edited.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `hoisted_audit_message` is meant to be reused by multiple destructive flows, consider moving it out of `nu::bootstrap` into a shared audit/confirm module so future callers don’t have to depend on bootstrap-specific code.
- Relying on exact `(audit)` strings for automation grep is quite brittle; you might want to introduce a small structured audit/logging helper (e.g., fixed tokens or fields around the prose) so the machine-readable contract is stable even if the human-facing text is later copy-edited.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR refactors the Nushell bootstrap/setup flow so the non-interactive (“non-TTY”) destructive-operation guard is consistently enforced via util::confirm::require_tty_or_yes, aligning bootstrap with the rest of the setup surface’s audit/log contract.
Changes:
- Route
src/nu/bootstrap.rsnon-TTY destructive guard throughrequire_tty_or_yesfor consistent audit output and behavior. - Extract a shared
hoisted_audit_message()formatter used by the hoisted-consent branch. - Add a golden-string test to pin the audit message text.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/nu/bootstrap.rs |
Replaces the inline non-TTY guard with require_tty_or_yes and factors audit text into hoisted_audit_message(). |
tests/setup_nu_test.rs |
Adds a golden test to pin the audit-trail message emitted when consent prompting is hoisted. |
Greptile SummaryThis PR is a symmetry cleanup that routes
Confidence Score: 5/5Safe to merge once the merge-order gate (PR #69 first) is respected; the refactoring is narrow and well-tested. The core logic change — replacing an inline TTY check with the shared helper — is straightforward and covered by new unit tests that verify both the refusal path and PATH immutability. The confirm_or_bail call in remove_managed_nu correctly threads the yes variable, so the double-prompt concern does not apply. The only flag is the committed one-shot Python patcher in scripts/, which is already stale but has no runtime impact. Files Needing Attention: scripts/apply_install_from_archive_fix.py — one-shot patcher committed after its target changes are already applied; not a runtime concern but may confuse future maintainers
|
| Filename | Overview |
|---|---|
| scripts/apply_install_from_archive_fix.py | One-shot patcher committed after its changes are already applied; fails with AssertionError if re-run |
| src/nu/bootstrap.rs | Routes non-TTY guard through require_tty_or_yes_with_tty, adds is_tty injection hook, and adds two tests covering the refusal path |
| src/util/confirm.rs | Adds hoisted_audit_message helper with a unit test; clean addition with golden test coverage |
| src/cmd/setup.rs | Adds require_tty_or_yes guard before remove_managed_nu's confirm_or_bail and populates new is_tty: None fields; yes parameter is correctly threaded through |
| tests/setup_nu_test.rs | Adds golden-string stability test for the hoisted audit message and updates struct initializers for the new is_tty field |
Reviews (4): Last reviewed commit: "fix(setup): gate register_existing_nu be..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/setup_nu_test.rs`:
- Around line 271-300: Add bootstrap-level coverage around
execute_nu_setup_with_installer for non-TTY input: introduce an injectable
TTY-detection seam, invoke the setup with yes=false and a simulated non-TTY, and
assert the installer is not called. Keep the existing
require_tty_or_yes_with_tty behavior and --yes coverage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 89b51d0d-717e-46de-9a17-08031619d704
📒 Files selected for processing (2)
src/nu/bootstrap.rstests/setup_nu_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Greptile Review
- GitHub Check: Sourcery review
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use
anyhow::Resultwith.context("what failed")for application code,thiserrorfor library types callers match on, and avoid panics in library code by returningResult.
**/*.rs: Use Rust 2021 edition conventions and match existing naming, module layout, and documentation level in edited files.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Error paths must returnanyhow::Resultwith context, and library code must not panic.
Unit tests must use injectable seams such asFakeCandidateRunnerand registrars; do not spawn realnuin unit tests.
Real-Nu acceptance tests should be marked#[ignore]and run separately when activation or nupm-import behavior changes.
Every new mutating code path must acquire the mutation lock and snapshot the lockfile before writes.
Mutating commands, includinginstall,remove,update,gc, andnupm import, must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must be written atomically usingwrite_json_atomic.
numan installmust write only to$NUMAN_ROOTand must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state are not authoritative.
Install payloads must use versioned, content-addressed paths and must never be overwritten in place.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.
The nupm boundary must remain read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Use Rust 2021 conventions; the crate's MSRV is...
Files:
tests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,toml}: All changes must pass formatting and linting:cargo fmtandcargo clippy -- -D warnings.
Behavior changes require corresponding tests, including relevant failure paths.
Files:
tests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when changing structure, conventions, or user-visible behavior, using
AGENTS.md,docs/, or command help as appropriate.
Files:
tests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
tests/setup_nu_test.rssrc/nu/bootstrap.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Keep unit tests inline with source modules and integration tests undertests/; test platform-specific code with mock platforms.
Real-Nu acceptance tests may be ignored and must be run withcargo test -- --ignored; unit tests should use injected fakes rather than a real Nu process.
Files:
tests/setup_nu_test.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Useanyhow::Resultfor application code andthiserrorfor library errors; add error context with.context(...)or?, and never panic in library code.
Use&Pathrather than&PathBufin function parameters.
Files:
src/nu/bootstrap.rs
🔍 Remote MCP GitHub Copilot
Relevant review context
- PR
#70is open with two commits:63afdadupdates the bootstrap guard;284772bextractshoisted_audit_messageand adds the golden test. - The shared helper inherited from
feature/numan-usehas distinct behavior: explicit--yesproceeds with an audit line, non-TTY without--yesfails, and interactive TTY proceeds to the normal prompt. Both branches have injectable tests. - Bootstrap now calls
require_tty_or_yes(options.yes, "Nushell setup")beforeconfirm_or_bail, preventing piped downloads without--yeswhile retaining the interactive prompt. - The formatter’s exact output is covered for
/usr/local/binand an empty path. - PR
#69remains open andblocked; PR#70’s description requires PR#69to merge first because the setup call sites are intentionally stacked. - PR
#70currently has no submitted reviews or review threads; all four automated review checks are still in progress.
🔇 Additional comments (3)
src/nu/bootstrap.rs (3)
222-223: LGTM!
443-458: LGTM!Also applies to: 485-485
735-740: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 284772baad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Shadow auto-approve: would auto-approve. Refactor routes an existing non-TTY guard through the shared require_tty_or_yes helper and adds a golden test for the hoisted audit message. The change is focused, behavior-preserving cleanup with no expansion of exposure or new operational tradeoffs.
Re-trigger cubic
Keep master's numan use / off-tree / install-active PATH behavior, PR 70's require_tty_or_yes bootstrap download guard and hoisted_audit_message. Route managed Nu removal through require_tty_or_yes, add an is_tty test seam with non-TTY installer refusal coverage, and make the audit golden test path-display portable. Co-authored-by: Anthony Thompson <github@trackdub.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nu/bootstrap.rs (1)
732-745: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEstablish the required mutation transaction before setup state changes.
These paths mutate PATH, active-version state, or the managed Nushell tree without a visible mutation lock and snapshot before the first write. Concurrent setup, removal, or replacement can interleave and leave the active marker or managed payload state inconsistent.
src/nu/bootstrap.rs#L732-L745: acquire the mutation lock and create the snapshot before the--yesalready-installed PATH update.src/nu/bootstrap.rs#L811-L827: retain the mutation lock through active-version persistence and persistent PATH updates.src/cmd/setup.rs#L461-L463: acquire the mutation lock and create the snapshot before clearing the active-version marker.src/cmd/setup.rs#L482-L487: acquire the same lock and snapshot before replacement clears state and deletes the managed tree.As per coding guidelines, “Every new mutating code path must acquire the mutation lock and snapshot the lockfile before writes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nu/bootstrap.rs` around lines 732 - 745, Establish the mutation transaction before every listed write: in src/nu/bootstrap.rs lines 732-745, acquire the mutation lock and snapshot the lockfile before the --yes already-installed PATH updates; in src/nu/bootstrap.rs lines 811-827, retain that lock through active-version persistence and persistent PATH updates; in src/cmd/setup.rs lines 461-463, acquire the lock and snapshot before clearing the active-version marker; and in src/cmd/setup.rs lines 482-487, do the same before replacement clears state or deletes the managed tree. Use the existing mutation-lock and snapshot mechanisms consistently across all four sites.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nu/bootstrap.rs`:
- Around line 486-490: Update register_existing_nu so that when
caller_consented_destructive is false, it invokes the TTY gate using
options.is_tty before confirm_or_bail, rejecting non-TTY execution unless
explicitly approved; preserve the existing confirmation flow for interactive
callers and ensure PATH mutation and active-version updates occur only after
this gate.
---
Outside diff comments:
In `@src/nu/bootstrap.rs`:
- Around line 732-745: Establish the mutation transaction before every listed
write: in src/nu/bootstrap.rs lines 732-745, acquire the mutation lock and
snapshot the lockfile before the --yes already-installed PATH updates; in
src/nu/bootstrap.rs lines 811-827, retain that lock through active-version
persistence and persistent PATH updates; in src/cmd/setup.rs lines 461-463,
acquire the lock and snapshot before clearing the active-version marker; and in
src/cmd/setup.rs lines 482-487, do the same before replacement clears state or
deletes the managed tree. Use the existing mutation-lock and snapshot mechanisms
consistently across all four sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db6f00a5-9ed0-469a-92c8-43664578bcc0
📒 Files selected for processing (3)
src/cmd/setup.rssrc/nu/bootstrap.rstests/setup_nu_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Greptile Review
- GitHub Check: Real-Nu acceptance (windows-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use
anyhow::Resultwith.context("what failed")for application code,thiserrorfor library types callers match on, and avoid panics in library code by returningResult.
**/*.rs: Use Rust 2021 edition conventions and match existing naming, module layout, and documentation level in edited files.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Error paths must returnanyhow::Resultwith context, and library code must not panic.
Unit tests must use injectable seams such asFakeCandidateRunnerand registrars; do not spawn realnuin unit tests.
Real-Nu acceptance tests should be marked#[ignore]and run separately when activation or nupm-import behavior changes.
Every new mutating code path must acquire the mutation lock and snapshot the lockfile before writes.
Mutating commands, includinginstall,remove,update,gc, andnupm import, must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must be written atomically usingwrite_json_atomic.
numan installmust write only to$NUMAN_ROOTand must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state are not authoritative.
Install payloads must use versioned, content-addressed paths and must never be overwritten in place.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.
The nupm boundary must remain read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Use Rust 2021 conventions; the crate's MSRV is...
Files:
src/cmd/setup.rssrc/nu/bootstrap.rstests/setup_nu_test.rs
src/{cmd,state,install}/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Call
create_snapshot()before mutations performed by install, update, remove, activate, deactivate, or nupm-import; garbage collection must treat payloads referenced by every snapshot as live roots.
Files:
src/cmd/setup.rs
src/cmd/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep clap subcommand handlers thin; delegate domain logic to
core/or installation logic toinstall/.
Files:
src/cmd/setup.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,toml}: All changes must pass formatting and linting:cargo fmtandcargo clippy -- -D warnings.
Behavior changes require corresponding tests, including relevant failure paths.
Files:
src/cmd/setup.rssrc/nu/bootstrap.rstests/setup_nu_test.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when changing structure, conventions, or user-visible behavior, using
AGENTS.md,docs/, or command help as appropriate.
Files:
src/cmd/setup.rssrc/nu/bootstrap.rstests/setup_nu_test.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
src/cmd/setup.rssrc/nu/bootstrap.rstests/setup_nu_test.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Useanyhow::Resultfor application code andthiserrorfor library errors; add error context with.context(...)or?, and never panic in library code.
Use&Pathrather than&PathBufin function parameters.
Files:
src/cmd/setup.rssrc/nu/bootstrap.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Keep unit tests inline with source modules and integration tests undertests/; test platform-specific code with mock platforms.
Real-Nu acceptance tests may be ignored and must be run withcargo test -- --ignored; unit tests should use injected fakes rather than a real Nu process.
Files:
tests/setup_nu_test.rs
🔍 Remote MCP DeepWiki, GitHub Copilot
Relevant review context
-
PR
#70’s actual diff includes an unrelatedscripts/apply_install_from_archive_fix.pyfile, despite the PR description claiming only four setup-related files. This script editssrc/nu/bootstrap.rsprogrammatically and should be reviewed for intentional inclusion. -
The shared helper’s documented contract is:
--yes: proceed and emit an audit line.- TTY without
--yes: allow the caller’s subsequent prompt. - Non-TTY without
--yes: emit an audit refusal and fail.
The new bootstrap call uses the injectablerequire_tty_or_yes_with_tty, matching this contract.
-
PR
#70is explicitly gated on PR#69, which supplies the other destructive setup call sites and targets the samerequire_tty_or_yesaudit behavior. Both PR#69and#70are currently open and blocked. -
PR
#71independently describes the broader setup architecture: destructive setup operations should require TTY/--yes, while version management uses active-version markers and journaled migration. This confirms that the reviewed changes sit within a larger, still-unmerged setup refactor. -
PR
#70’s current checks show successful formatting, Clippy, package, Linux/macOS tests, and real-Nu acceptance on Linux/macOS. Windows tests, Windows real-Nu acceptance, Rust analysis, and Greptile review were still in progress; CodeQL completed with a neutral conclusion. -
DeepWiki could not locate the referenced files or helper in its indexed repository state, so it provides no reliable architectural validation for this PR.
🔇 Additional comments (3)
src/nu/bootstrap.rs (1)
5-5: LGTM!Also applies to: 210-210, 312-312, 461-477, 680-684, 716-731, 748-756, 769-773, 1027-1027, 1058-1082
tests/setup_nu_test.rs (1)
12-12: LGTM!Also applies to: 41-41, 55-76, 283-311
src/cmd/setup.rs (1)
1-1: LGTM!Also applies to: 221-249, 293-293, 342-348, 369-371, 406-412, 431-434, 448-451, 800-820, 888-912, 991-992
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Keep the (audit) string contract; share the formatter from util::confirm so other destructive hoist surfaces need not depend on Nu bootstrap. Co-authored-by: Cursor <cursoragent@cursor.com>
When consent is not hoisted, refuse non-TTY PATH/active-version updates without --yes using options.is_tty, matching other destructive setup paths. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nu/bootstrap.rs (1)
486-490:⚠️ Potential issue | 🟠 MajorKeep direct registration behind the shared consent gate.
When
caller_consented_destructiveisfalse,register_existing_nustill reachesconfirm_or_bailwithoutrequire_tty_or_yes_with_tty. Becauseconfirm_or_bailauto-confirms non-TTY input, direct registration andsetup nu useor doctor-repair paths can mutate PATH without a TTY or--yes.Unless PR
#69adds an outer gate for every caller, call the shared gate beforeconfirm_or_bailand useoptions.is_ttywhen it is set. This is the same unresolved finding from the previous review.#!/bin/bash set -euo pipefail rg -n -C 8 \ 'register_existing_nu|caller_consented_destructive|require_tty_or_yes(_with_tty)?' \ src tests🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nu/bootstrap.rs` around lines 486 - 490, Ensure register_existing_nu routes every non-consented destructive registration through require_tty_or_yes_with_tty before confirm_or_bail, including direct CLI and doctor-repair callers. When options.is_tty is set, pass that value to the shared gate; preserve the existing caller_consented_destructive bypass for callers already gated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/setup_nu_test.rs`:
- Around line 275-283: Align register_existing_nu_audit_text_is_stable and its
documentation with the actual hoisted_audit_message formatter contract, since
the test does not exercise register_existing_nu. Alternatively, add a focused
test that invokes register_existing_nu and verifies its emitted audit output,
while retaining a separate formatter test if needed.
---
Outside diff comments:
In `@src/nu/bootstrap.rs`:
- Around line 486-490: Ensure register_existing_nu routes every non-consented
destructive registration through require_tty_or_yes_with_tty before
confirm_or_bail, including direct CLI and doctor-repair callers. When
options.is_tty is set, pass that value to the shared gate; preserve the existing
caller_consented_destructive bypass for callers already gated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e78ef47e-d214-4c4a-ae2e-6d3429a2bc53
📒 Files selected for processing (4)
src/cmd/setup.rssrc/nu/bootstrap.rssrc/util/confirm.rstests/setup_nu_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Greptile Review
- GitHub Check: Test (windows-latest)
- GitHub Check: Real-Nu acceptance (windows-latest)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Use Serena's semantic, symbol-aware tools as the primary tools for reading and editing code; use built-in Read, Glob, Grep, and Edit only under the stated exceptions.
Before editing a code file, inspect its symbol overview, read the specific symbols being changed, and edit them with Serena's symbol-aware tools.
Understand the existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, unnecessary error handling, feature flags, and compatibility shims.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a 2–3 sentence recommendation with the main tradeoff and do not implement until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Address security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, force-pushing, modifying CI/CD, posting externally, or uploading content.
When blocked, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration before deleting them.
Only commit when explicitly asked; do not proactively update git configuration or push changes.
Do not skip Git hooks unless explicitly asked; if a pre-commit hook fails, fix the issue, re-stage, and create a new commit rather than amending.
Stage files by name rather than using git add -A or git add ., to avoid accidentally including secrets or large binaries.
Use a HEREDOC for commit messages, and add co-author attribution only when the user explicitly requests the exact trailer.
Do not force-pus...
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Add comments only when the WHY is non-obvious; do not narrate what the code does, reference the current task, or reference the PR in comments.
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
!**/.env,!**/credentials.json,!**/*.pem
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit files that appear to contain secrets, including .env, credentials.json, and PEM files; warn before doing so even if explicitly requested.
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Useanyhow::Resultfor application code,thiserrorfor library error types, add context with.context(...)or?, and never panic in library code; return errors instead.
Use&Pathrather than&PathBufin function parameters.
Add unit tests inline with source modules and integration tests undertests/; test-first development is expected, and platform-specific code must be tested with mock platforms.
**/*.rs: Use the Rust 2021 edition.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Library code must not panic; error paths should returnanyhow::Resultwith context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock viaacquire_mutation_lock(root)and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must usewrite_json_atomic.
numan installmust write only to$NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state must not be authoritative.
Install payloads under versioned, content-addressed paths and never overwrite them in place.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.
**/*.rs: All CI gates must pass:cargo test,cargo clippy -- -D warnings, andcargo fmt --check.
Every mutating command—includinginstall,remove,update,gc, and futurenupm import—must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must use `write_js...
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use dependency conventions:
serde/serde_jsonfor JSON,tomlfor configuration,clapderive macros for CLI parsing, andanyhow/thiserroraccording to application versus library boundaries.
Files:
src/cmd/setup.rssrc/util/confirm.rssrc/nu/bootstrap.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*.{rs,nu}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,nu}: Real-Nu acceptance tests must be marked#[ignore]and should be run when changes affect activation or nupm import; unit tests must not spawn realnuand should use injectable seams such asFakeCandidateRunneror registrars.
The nupm integration must be read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Unit tests must use
FakeCandidateRunneror injectable registrars and must not spawn a realnuprocess.
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Match existing naming, module layout, and documentation level in the file being edited; update
AGENTS.md,docs/, or command help when structure, conventions, or user-visible behavior changes.Tests must cover failure modes, not only successful execution.
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's established serialization and module conventions rather than introducing unrelated refactors.
Files:
src/cmd/setup.rssrc/util/confirm.rstests/setup_nu_test.rssrc/nu/bootstrap.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use injected fakes and runners for activation-related unit tests; do not spawn a real Nu binary in unit tests.
Files:
tests/setup_nu_test.rs
🔍 Remote MCP DeepWiki, GitHub Copilot
Relevant review context
-
The PR’s diff includes an unrelated
scripts/apply_install_from_archive_fix.pyscript that modifiessrc/nu/bootstrap.rsto use versioned installation paths. It is not listed in the PR description and uses bareassertchecks, making its inclusion worth clarifying. -
An earlier review identified that direct
register_existing_nucallers could still bypass the TTY gate; that comment is now outdated, but the current diff still only adds the gate to bootstrap setup and managed removal. Verify whether direct registration is intentionally covered by the prerequisite PR#69or requires an explicit guard here. -
The golden audit test now constructs expected output with
parent.display(), addressing the prior cross-platform path concern. However, it still reconstructs the format string instead of comparing against a literal, so it verifies consistency with the helper implementation more than protection against accidental wording changes. -
PR
#70is blocked and explicitly depends on PR#69. PR#69introduces the remaining destructive setup call sites and the broader journaled migration flow. -
Current checks: formatting, Clippy, package, MSRV, Ubuntu tests, Ubuntu/macOS real-Nu acceptance, and action/Python analysis passed. Windows tests, Windows real-Nu acceptance, macOS tests, Rust analysis, and Greptile review were still in progress; CodeQL completed with a neutral conclusion.
-
DeepWiki could not locate the relevant
numanfiles, so it provided no reliable architectural validation.
🔇 Additional comments (4)
src/util/confirm.rs (1)
90-106: LGTM!Also applies to: 148-158
src/nu/bootstrap.rs (1)
24-35: LGTM!Also applies to: 457-459, 680-684, 735-739, 769-786, 1028-1028, 1058-1083
tests/setup_nu_test.rs (1)
70-70: LGTM!Also applies to: 284-310
src/cmd/setup.rs (1)
11-22: LGTM!Also applies to: 257-257, 367-378, 430-441, 458-472, 854-874
Take #70 injectable is_tty / require_tty_or_yes_with_tty for download and already-installed setup paths. Keep PR 69 refuse-in-place --force behavior and both sides' non-TTY regression tests. Co-authored-by: Anthony Thompson <github@trackdub.com>
Summary
Symmetry-cleanup follow-up to PR #67 (
feat: implement numan use for side-by-side Nu version managementonfeature/numan-use) and PR #69 (pr-migrate-legacy-installs).Every destructive setup entry now flows through one
require_tty_or_yeshelper, with one audit pattern, insrc/util/confirm.rs. Bootstrap's download/install path was the last surviving inline duplicate of the rule — this PR routes it through the helper so safe-batch automation can grep one consistent(audit)prefix across the whole setup surface instead of a mixed(audit)/Nushell setup cancelled.pair.This PR must merge after PR #69 lands — the helper is already in
feature/numan-use, but the other two destructive call sites it unifies live onpr-migrate-legacy-installs. Merging PR #67 first (i.e. this PR) would leave bootstrap as the only consumer and PR #69 would still be working against an old confirm.rs shape.Three destructive-setup call sites (after this PR lands)
src/nu/bootstrap.rs::execute_nu_setup_with_installer(Nushell download + PATH mutation)if !options.yes && !std::io::stdin().is_terminal() { bail!("Nushell setup cancelled."); }crate::util::confirm::require_tty_or_yes(options.yes, "Nushell setup")?;src/cmd/setup.rs::execute_use_existing/execute_use_path(off-path registration + managed-tree deletion)require_tty_or_yes(PR #69 batch 1)src/cmd/setup.rs::remove_managed_nu(numan setup nu removedestructive variant)require_tty_or_yes(PR #69 batch 1)Audit-text guaranteed identical:
(audit) explicit --yes accepted for {what}; proceeding without interactive prompt.for the--yespath and(audit) implicit non-TTY session; refusing destructive {what} without --yes.for the non-TTY rejection path. Thewhatlabels are:#1→"Nushell setup"(this PR)#2→"off-path registration"(PR [phase-1] cleanup: journaled legacy migration #69)#3→"managed Nushell removal"(PR [phase-1] cleanup: journaled legacy migration #69)Diff stat (vs.
feature/numan-use)Two commits on the branch:
63afdadrefactor(setup): route bootstrap non-TTY guard through require_tty_or_yes284772brefactor(setup): extract hoisted audit message helper + golden testThe second commit extracts
pub fn hoisted_audit_message(parent: &Path) -> Stringinbootstrap.rsand pins its literal output via the new golden testtests/setup_nu_test.rs::register_existing_nu_audit_text_is_stable. Bothrequire_tty_or_yesandhoisted_audit_messagewere already infeature/numan-use; this PR closes the bootstrap-side callers and locks the audit text so a future copy-edit can't surface four divergent variants.Validation log
cargo buildcargo fmt --checkcargo clippy -- -D warningscargo test --libcargo test --test setup_nu_testregister_existing_nu_audit_text_is_stable)cargo test --tests-- --ignoredCI acceptance job)Merge order
If the gate is respected, the merge sequence is:
feature/numan-use(introduces the tworequire_tty_or_yescallers incmd/setup.rs)feature/numan-use(closes the third caller inbootstrap.rs)feature/numan-use→master(the eventual feature delivery)Breaking the gate is recoverable but messy: PR #69 would re-introduce a divergent inline audit pattern in the meantime and require a follow-up rebase. The two PRs do not conflict on
bootstrap.rsbut they do conflict on the audit-trail contract.Refs
feat: implement numan use for side-by-side Nu version management(feature/numan-use)phase-1: cleanup: journaled legacy migration(pr-migrate-legacy-installs)docs/active-plugin-gate.md— the active-update invariant this refactor pulls in line withsrc/util/confirm.rs::require_tty_or_yes_with_tty(yes, what, is_tty)— the helper both PRs converge on🤖 Generated with Codebuff
Co-Authored-By: Codebuff noreply@codebuff.com