feat(try): update starter selection for expanded 0.114 catalog - #111
feat(try): update starter selection for expanded 0.114 catalog#111tonythethompson wants to merge 65 commits into
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.
- Pull 251 lines of migration fs (Legacy*Detector type alias, LegacyPostCreateHook type alias, detect_legacy_version, migrate_legacy_install, migrate_legacy_install_with_detector, parse_nu_version_from_output) plus 8 migrate_legacy_* tests + create_legacy_binary helper + production_detector_prefers_version_metadata_file + test_parse_nu_version_from_output into a new crate::nu::migrate_legacy module. - Add 'pub(crate)' to write_active_marker (now consumed by crate::nu::migrate_legacy). - Retarget use_cmd.rs to call crate::nu::migrate_legacy::migrate_legacy_install. - Re-register the module in src/nu/mod.rs. This is the pr-migrate-legacy-installs half of the split: phase 1 cleanup + journaled transaction for legacy single-binary -> versioned installation with self-healing and numan doctor --fix reconciliation. Co-authored-by: Codebuff <noreply@codebuff.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Threads addressed (changed in code):
* chatgpt PR69 S1A src/cmd/doctor.rs
doctor --fix auto-repair branch reacquires the root mutation lock
before invoking migration_journal::reconcile, so a concurrent numan use
cannot race the same Prepared -> Renamed transition.
* copilot PR69 VwSra src/state/migration_journal.rs
PendingMigration::load now hard-fails on unknown schema_version
(no longer silently coerces); a future journal variant cannot be
misinterpreted as the current one.
* cubic PR69 UzV src/cmd/use_cmd.rs
numan use latest preserves an existing off-tree binary_path when the
selected version matches the existing marker (the previous write
dropped binary_path, breaking resolution of 'setup nu use <path>'
picks).
* chatgpt PR69 S08 src/cmd/setup.rs
Both setup nu use <path> callsites now write_active_version_with_binary
after register_existing_nu succeeds, so numan use list reports the
registered off-tree binary as the active selection.
* chatgpt PR69 S09 src/cmd/setup.rs
remove_managed_nu and remove_managed_nu_if_present clear the
active-version marker at the top of the function, before deleting the
managed tree, so the marker cannot dangle at a binary just removed.
* cubic PR69 UzU src/nu/migrate_legacy.rs
parse_nu_version_from_output delegates to core::nu_version::NuVersion
::parse, which already strips build-hash suffixes ('0.113.1 (abc123)').
Manually-placed legacy binaries whose --version includes a build hash
migrate cleanly; bare '0.113.1' falls through to the semver-only path.
* cubic PR69 UzM src/nu/paths.rs
find_nu_executable_with_root now propagates read_active_version errors
instead of silently treating them as 'no marker'. A present-but-malformed
active-version marker surfaces loudly so numan init / setup loader
cannot silently fall back to PATH Nu.
* cubic PR69 UzG src/nu/migrate_legacy.rs
migrate_legacy_install_with_detector refuses to scan / mutate under a
symlinked managed directory; the rename or filesystem-truth cleanup
cannot redirect outside $NUMAN_ROOT.
Build / clippy / fmt / lib tests (cargo test --lib -> 468 passed; 0
failed).
Deferred (kept as Discussion for the user):
* qodo S26 — anyhow -> thiserror migration in public APIs (large refactor)
* chatgpt S05 — refresh cached NuPaths when switching versions (architectural)
* chatgpt S06 — preserve ~/.local/bin/nu symlink through migration (UX)
* cubic UzO — HOME-mutating test seam isolation (test infra refactor)
* chatgpt VwS04 — install pinned releases to versioned layout (overlaps
PR67 VpJrb on the other branch; left for cross-PR consolidation)
* outdated=True threads: VwSrT, VwSrk, VwS1C (skip per skill rules)
Per user instruction: push to PR branch only; do NOT merge the PR.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
…ate cfg(test) read_active_version After the rebase of pr-migrate-legacy-installs onto current master, the detection function was missing its closing bracket (rustc reported an 'unclosed delimiter' at detect_legacy_version that reached EOF), and cargo clippy -- -D warnings surfaced three warnings on lib build: - src/nu/bootstrap.rs:5 — std::io::IsTerminal imported but unused now that the IsTerminal check moved to src/util/confirm.rs. - src/nu/migrate_legacy.rs:19 — read_active_version imported at module scope but only used inside #[cfg(test)]. Restrict to #[cfg(test)] so the lib build is clean and tests still reach it via a sibling import inside the tests module. - src/cmd/doctor.rs:1342 — value assigned to lock was never read; the reacquire from the chatgpt PR69 S1A path is the canonical call. Drop the redundant 'lock = Some(...)' line. Also re-runs cargo fmt to absorb any drift carried in by the rebase. Validation: cargo build OK; cargo clippy -- -D warnings clean; cargo test --lib 468 passed / 0 failed on pr-migrate-legacy-installs.
Compute managed_dir once and assert_not_symlink before any reconcile stage recovery, including Active. Add regression coverage for a symlinked tools/nushell tree, and assert that `numan use list` creates no snapshot while switch/latest create a PreMutation snapshot.
Propagate invalid active-marker versions, migrate legacy before `numan use list`, preserve existing active selection on migration, and gate real-Nu setup tests behind #[ignore]. Co-authored-by: Cursor <cursoragent@cursor.com>
Only preserve binary_path when it is non-empty and still a file, so stale markers cannot clobber a valid on-tree latest selection. Co-authored-by: Cursor <cursoragent@cursor.com>
Re-probe and rewrite nu_state/paths.json after numan use, or clear it when probing fails, so activate cannot keep using a previous side-by-side Nu. Co-authored-by: Cursor <cursoragent@cursor.com>
Serialize PATH-mutating ignored setup tests via PathRestoreGuard now exported from the library so parallel acceptance workers cannot clobber each other's process PATH. Co-authored-by: Cursor <cursoragent@cursor.com>
Mark the usage example as text so Real-Nu `cargo test -- --ignored` does not compile it as a rustdoc ignore-doctest. Co-authored-by: Cursor <cursoragent@cursor.com>
Ignore active_nu_binary errors when resolving the already-installed binary for latest setup so a stale marker cannot block fallback to the newest on-tree install. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve conflicts between the setup-subcommand-lock branch and master: - setup.rs: keep branch's versioned-layout use/remove logic (force gate, version-before-mutation, error-context clears) and add master's pre-mutation snapshots. - bootstrap.rs: keep branch's flow-aware already-installed short-circuit (dangling-marker fallback) and add master's snapshot; adopt master's is_tty seam for the non-TTY guard; keep both test sets. - doctor.rs: adopt master's redesigned DoctorArgs (scan/json) and map the branch's migration-journal tests onto it; keep master's fail-closed off-path repair (no --yes). - confirm.rs/hints.rs/mod.rs: auto-merged. - model.rs: deactivate drops --yes (master removed the flag); remove keeps --yes (branch's fail-closed removal gate). - setup_nu_test.rs: keep branch's --force gate tests and master's audit-text golden test; doctor_test.rs: adapt use_existing calls to 3-arg signature.
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>
…map-contract/v1
The roadmap-drift CI workflow in numan, numan-plugins, and numan-registry
used to fetch the consolidated roadmap + drift script from
'raw.githubusercontent.com/.../numan/master/...'. A push to master could
silently change the guardrail; sibling roadmap files drifted independently
(PR 67 had to do a 'cross-repo audit' pass to undo 'numan use >= stub'
claims after the feature shipped).
This commit freezes the cross-repo guardrail at a versioned tag.
- docs/contracts/roadmap-v1.md -- the freeze doc explaining what v1
freezes, the sentinel rules, and the bump procedure.
- scripts/bump-contract.sh -- the only sanctioned way to bump to vN>1.
Validates drift locally, refreshes the pinned SHA in three workflow
yml files, pushes the new tag, opens three coordinated PRs.
- .github/workflows/ci.yml, cross-repo-mirror/{numan-plugins,
numan-registry}/.github/workflows/roadmap-drift.yml -- pinned at
CONTRACT_TAG=numan-roadmap-contract/v1 / CONTRACT_SHA=<this commit>.
Adds a 'Verify pinned SHA still resolves to tag' step that fails
closed if a force-pushed tag moves.
- cross-repo-mirror/README.md -- updated to instruct contributors to
pin at the contract SHA, not @master.
Initial v1 (--init) was published from a pre-commit dry-run; that
tag pointed at SHA 99aa695 without the freeze infra and has been
deleted. v1 will be recreated at this commit so the tag SHA matches the
freeze infra.
The numan-roadmap-contract/v1 tag was created at freeze commit f220940, but the workflow yml files on the same commit pinned CONTRACT_SHA to 99aa695 (the pre-freeze SHA from feature/numan-use HEAD), because that SHA was the only known-stable value at freeze time. The verify-SHA step in every CI job therefore compares the tag (resolved f220940) against the env (99aa695) and fails closed. This follow-up commit rewrites CONTRACT_SHA to f220940 across the four workflow yml files, the cross-repo-mirror README, and the contract doc. The change is in lockstep with a tag force-move so the v1 contract is internally consistent: the tag points to commit f220940, and every workflow yml now names f220940 as its CONTRACT_SHA. URL fetches via $CONTRACT_SHA resolve to f220940's content, which contains the contract doc + drift script + consolidated-multi-repo-roadmap.md that v1 freezes. Validation: $CONTRACT_SHA in ci.yml, numan-plugins, numan-registry, and the README's copy-paste example all match $git rev-parse numan-roadmap-contract/v1^{commit} = f220940.
Earlier freeze revisions pinned both CONTRACT_TAG and a literal CONTRACT_SHA in the yml env. The intent was 'tag-resolved SHA == yml-pinned SHA' to detect force-pushes. But the freeze commit can't self-reference its own SHA without a chicken-and-egg followup commit (ad infinitum); and the literal pin would always be one tag-move behind the frozen tag. Trading the comparison for a tag-existence verification loses force-push detection but gains a single-source-of-truth pattern: the tag itself is the contract. The bump script (scripts/bump-contract.sh) is still the only sanctioned way to create or move a tag, and it explicitly refuses to clobber. So the tag is effectively immutable from the maintainer side. CI fails if the tag disappears; CI succeeds if it points anywhere, because the content is versioned in the tag history.
Align sibling CONTRACT_SHA with the live v1 tag, fix INIT/version tag computation and fail-closed drift preflight in bump-contract.sh, and harden mirror dry-run / install docs without diverging pinned contract artifacts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Anthony Thompson <github@trackdub.com>
Pop deferred headings correctly in check-roadmap-drift, pin numan CI to an immutable CONTRACT_SHA (peeling annotated tags), rewrite bump-contract to commit pins, materialize sibling branches, and keep mirror docs/smoke-tests aligned with the SHA pin. Co-authored-by: Anthony Thompson <github@trackdub.com>
Point CONTRACT_SHA and sibling blob links at the content freeze that includes the drift-script and catalog markdown fixes. Co-authored-by: Anthony Thompson <github@trackdub.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Pass GITHUB_TOKEN to curl and the annotated-tag peel so shared runners avoid unauthenticated rate limits on the roadmap pin check. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add nu_plugin_explore (interactive TUI data browser) as 2nd choice after skim - Add nu_plugin_highlight (syntax highlighting) as visual starter - Keep nu_plugin_skim as primary pick (fuzzy finder, best demo) - Remove stale abusch/nu_plugin_semver (stuck at 0.113, no 0.114 release) - Add highlight as universal fallback (any Nu version) - Add usage hints for explore and highlight plugins Selection priority for Nu 0.114: 1. skim (interactive fuzzy finder) 2. explore (TUI data browser) 3. highlight (syntax highlighting) 4. nutest (test framework) Fallback for any Nu: 1. highlight 2. nutest
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThe change introduces an immutable roadmap contract workflow and a journaled Nushell migration system. It also adds typed version-management errors, mutation locks, non-interactive confirmation gates, safer setup behavior, cache refresh handling, and related tests. ChangesRoadmap contract coordination
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Closing: the STARTERS array and auto-selection logic were removed from master in a recent refactor. |
PR Summary by QodoRefactor
AI Description
Diagram
High-Level Assessment
Files changed (34)
|
Greptile SummaryThis PR expands
Confidence Score: 4/5The starter and migration changes appear sound, but the contract-bump workflow should be fixed before merging because future coordinated bump PRs cannot pass their required tag verification. The bump script intentionally withholds the new remote tag until after merge while every generated PR requires that tag to exist during CI, making the documented contract advancement path fail deterministically. Files Needing Attention: scripts/bump-contract.sh and the roadmap-drift workflows
|
| Filename | Overview |
|---|---|
| src/cmd/try_cmd.rs | Updates the curated 0.114 starter order and adds explore/highlight usage hints; no concrete defect was established. |
| src/nu/migrate_legacy.rs | Adds a guarded, journaled migration from the flat Nu binary layout to versioned directories with recovery for interrupted renames. |
| src/state/migration_journal.rs | Implements Prepared/Renamed/Active reconciliation with filesystem checks, traversal validation, and retained journals on failed recovery. |
| src/nu/version_manager.rs | Adds typed errors, off-tree binary paths, stricter version validation, and legacy-layout listing support. |
| scripts/bump-contract.sh | Coordinates contract pins and PR creation, but defers publishing the tag that those PRs require during CI. |
| .github/workflows/ci.yml | Pins roadmap artifacts and verifies the tag-to-SHA mapping, which exposes the bump script's unpublished-tag ordering failure. |
Prompt To Fix All With AI
### Issue 1
scripts/bump-contract.sh:301-304
**Unpublished tag blocks contract bumps**
When a maintainer runs the contract-bump workflow, the script pushes the Numan and sibling PR branches but defers publishing the new tag until after merge. Each PR's roadmap-drift workflow immediately requires that remote tag to resolve to `CONTRACT_SHA`, so all coordinated bump PRs fail CI and cannot reach the merge after which the tag is supposed to be published.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(try): update starter selection for ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ "$DRY_RUN" -eq 0 ]; then | ||
| log "pushing branch $BRANCH_NAME (HEAD) to numan" | ||
| git push origin "HEAD:refs/heads/$BRANCH_NAME" | ||
| log "NOT pushing $NEW_TAG yet — publish with: git push origin refs/tags/$NEW_TAG after the numan PR merges" |
There was a problem hiding this comment.
When a maintainer runs the contract-bump workflow, the script pushes the Numan and sibling PR branches but defers publishing the new tag until after merge. Each PR's roadmap-drift workflow immediately requires that remote tag to resolve to CONTRACT_SHA, so all coordinated bump PRs fail CI and cannot reach the merge after which the tag is supposed to be published.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/bump-contract.sh
Line: 301-304
Comment:
**Unpublished tag blocks contract bumps**
When a maintainer runs the contract-bump workflow, the script pushes the Numan and sibling PR branches but defers publishing the new tag until after merge. Each PR's roadmap-drift workflow immediately requires that remote tag to resolve to `CONTRACT_SHA`, so all coordinated bump PRs fail CI and cannot reach the merge after which the tag is supposed to be published.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![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: 4dbf830a42
ℹ️ 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".
| // `numan use list` and downstream activation see it as the selected Nu. | ||
| // `latest` is left to `numan use latest` — the user explicitly resolves the | ||
| // release tag there. | ||
| if let Some(version) = &options.version { |
There was a problem hiding this comment.
When a fresh numan setup nu installs the latest release, options.version is None, so this condition skips the active-marker write entirely. The command consequently succeeds with no authoritative selection: numan use list shows no active release and later resolution can choose a version by directory ordering rather than the installed release. Derive the concrete version from installed and write the marker for both pinned and latest installs.
AGENTS.md reference: AGENTS.md:L148-L148
Useful? React with 👍 / 👎.
| }) | ||
| .unwrap_or_else(|| dest.clone()) | ||
| }; | ||
| if options.yes { |
There was a problem hiding this comment.
When any managed version already exists and the caller runs bare numan setup nu --yes, this branch returns the existing binary immediately instead of accepting the implied “Reinstall latest release?” prompt. As a result, the documented non-interactive way to skip confirmation can never upgrade an existing installation to the current latest release unless the caller additionally knows to pass --force; --yes should change prompting, not the requested operation.
AGENTS.md reference: AGENTS.md:L239-L239
Useful? React with 👍 / 👎.
| if args.version == "list" { | ||
| let _lock = acquire_mutation_lock(root)?; | ||
| crate::nu::migrate_legacy::migrate_legacy_install(root) | ||
| .with_context(|| "Failed to migrate legacy Nu installation before list")?; |
There was a problem hiding this comment.
For a legacy flat install, numan use list now calls a migration that renames the managed binary and can write active-version.json, but this branch bypasses the create_snapshot call used immediately below for the other mutating arms. Thus a nominal listing performs state mutation without the required pre-mutation recovery point; either keep listing read-only or snapshot before invoking the migration.
AGENTS.md reference: AGENTS.md:L159-L159
Useful? React with 👍 / 👎.
| pkg("tonythethompson/nu_plugin_explore", ">=0.114.0 <0.115.0", true), | ||
| pkg("cptpiepmatz/nu_plugin_highlight", ">=0.114.0 <0.115.0", true), |
There was a problem hiding this comment.
Running the repository's formatting gate, cargo fmt --all -- --check, fails on this call and the two analogous new pkg(...) calls below because rustfmt expands their arguments. The mandatory Format CI job therefore fails for this commit; apply cargo fmt before merging.
AGENTS.md reference: AGENTS.md:L28-L33
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if args.version == "list" { | ||
| let _lock = acquire_mutation_lock(root)?; | ||
| crate::nu::migrate_legacy::migrate_legacy_install(root) | ||
| .with_context(|| "Failed to migrate legacy Nu installation before list")?; |
There was a problem hiding this comment.
1. use list acquires mutation lock 📘 Rule violation ≡ Correctness
numan use list is required and described as read-only, but it currently acquires the root mutation lock and runs migrate_legacy_install, which can create directories, rename binaries, and write the active-version marker. Because this happens before the pre-mutation snapshot path, a pure listing operation can unexpectedly change state and also fail or block due to lock contention without any rollback baseline.
Agent Prompt
## Issue description
`numan use list` must be strictly non-mutating: it should not acquire the root mutation lock, and it must not perform legacy migration or any filesystem writes (e.g., directory creation, binary rename, active-version marker updates). The current implementation takes the mutation lock and runs legacy migration on the list path, and it returns before the pre-mutation snapshot creation, so state can change during what is still described and tested as a read-only listing operation.
## Issue Context
Compliance (PR Compliance ID 2486203) requires `numan use list` to be read-only so it cannot unexpectedly modify user state or block/fail due to mutation lock contention. Additionally, repository review/architecture rules require that any mutations follow the same pre-mutation snapshot discipline as other mutating `use` operations; currently migration occurs before the snapshot path and the existing test only asserts that `list` creates no snapshot (it does not prove that `list` performs no mutation).
## Fix Focus Areas
- src/cmd/use_cmd.rs[26-59]
- src/nu/migrate_legacy.rs[237-325]
- src/cmd/use_cmd.rs[253-275]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| pub fn reconcile(root: &Path) -> Result<Option<PendingMigration>> { | ||
| let Some(journal) = PendingMigration::load(root)? else { | ||
| return Ok(None); | ||
| }; |
There was a problem hiding this comment.
2. reconcile doesn’t emit migration_invalid 📘 Rule violation ≡ Correctness
On unreadable/invalid migration-journal.json, PendingMigration::load errors and reconcile propagates the error instead of emitting journal.migration_invalid and leaving the journal untouched. This breaks the required guarded self-healing behavior and turns a corrupt journal into a hard failure path.
Agent Prompt
## Issue description
`migration_journal::reconcile` currently fails hard when the journal cannot be read or parsed. Compliance requires emitting a `journal.migration_invalid` event (Error / Manual repair semantics) and returning without modifying/deleting the journal or attempting auto-reconciliation.
## Issue Context
`migrate_legacy_install_with_detector` calls `migration_journal::reconcile(root)?;` early; a corrupt journal should not crash this flow, and the invalid-journal condition must be surfaced via the required telemetry/event.
## Fix Focus Areas
- src/state/migration_journal.rs[126-150]
- src/state/migration_journal.rs[211-214]
- src/nu/migrate_legacy.rs[164-169]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "Migration journal at '{}' has unsafe version component '{}'. \ | ||
| Refusing to reconcile to avoid escaping the managed tree. \ | ||
| Run `numan doctor --fix` to discard the journal.", | ||
| PendingMigration::journal_path(root).display(), |
There was a problem hiding this comment.
7. Unsafe journal fix cannot work 🐞 Bug ≡ Correctness
The unsafe-version rejection tells users to run numan doctor --fix, but doctor classifies the parseable journal as auto-repairable and calls the same rejecting reconcile function. The suggested command records another failure and leaves the journal in place.
Agent Prompt
## Issue description
Make the recovery hint match actual behavior. Either classify unsafe version components as invalid/manual with an explicit deletion instruction, or safely implement doctor-side discard without passing the value into path construction.
## Issue Context
`PendingMigration::load` validates schema and JSON but not the version component, so doctor publishes the Auto-tier pending finding and later invokes `reconcile`, which deterministically rejects it.
## Fix Focus Areas
- src/state/migration_journal.rs[211-228]
- src/cmd/doctor.rs[650-683]
- src/cmd/doctor.rs[1445-1476]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Syntax highlighting — immediate visual payoff, works on any data | ||
| StarterSpec { | ||
| id: "cptpiepmatz/nu_plugin_highlight", | ||
| nu_minor: Some((0, 114)), | ||
| os: None, | ||
| }, | ||
| // Test framework — practical for all users | ||
| StarterSpec { | ||
| id: "vyadh/nutest", | ||
| nu_minor: Some((0, 114)), | ||
| os: None, | ||
| }, | ||
| // Universal fallbacks (any Nu version) | ||
| StarterSpec { | ||
| id: "cptpiepmatz/nu_plugin_highlight", | ||
| nu_minor: None, | ||
| os: None, | ||
| }, |
There was a problem hiding this comment.
9. Redundant duplicate highlight starter entry 🐞 Bug ⚙ Maintainability
STARTERS now lists cptpiepmatz/nu_plugin_highlight twice: once pinned to nu_minor: Some((0,114)) and once as a universal fallback (nu_minor: None). In select_starter's second pass, package lookup is done purely by spec.id (packages.iter().find(|p| p.id.to_string() == spec.id)), ignoring nu_minor, so the pinned entry already resolves the same package with the same compatibility result before the universal fallback entry is ever reached, making the second entry dead/redundant in that pass.
Agent Prompt
## Issue description
The STARTERS const array contains two entries for `cptpiepmatz/nu_plugin_highlight`: one pinned to Nu 0.114 and one universal (`nu_minor: None`). Because `select_starter`'s fallback pass (pass 2) looks up the package purely by id and ignores `nu_minor`, the pinned entry already produces the same match/compatibility result as the universal entry, making the second occurrence redundant/dead in that pass.
## Issue Context
`select_starter` in `src/cmd/try_cmd.rs` iterates `STARTERS` in three passes: (1) exact OS+Nu-minor match, (2) any curated starter compatible regardless of minor (id-only lookup), (3) suggest a pin. The duplicate highlight spec was added to express 'skim -> explore -> highlight -> nutest for 0.114' and 'highlight -> nutest as universal fallback', but the fallback's dependency on id-only lookup in pass 2 makes the second entry a no-op for selection purposes.
## Fix Focus Areas
- src/cmd/try_cmd.rs[38-74]
- src/cmd/try_cmd.rs[221-232]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if managed_dir_was_present && !force { | ||
| bail!( | ||
| "Refusing `numan setup nu path` while a managed Nushell install at '{}' exists.\n\n\ | ||
| The destructive two-step flow (delete the managed tree + adopt PATH Nu) would \ | ||
| discard every installed version and the active-version marker. Re-run with \ | ||
| `--force` to opt into it, or run `numan setup nu remove` first to stage the \ | ||
| removal out-of-band so this subcommand can register PATH Nu without \ | ||
| touching managed state.\n\n\ | ||
| Both flows are reversible only by `numan setup nu <version>`.", | ||
| managed_dir.display(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
10. Setup nu now requires new --force flag 🐞 Bug ≡ Correctness
execute_use_path and execute_use_existing in src/cmd/setup.rs now bail immediately with a hard error whenever a managed Nushell install exists and the new --force flag is not passed, even when the caller supplies --yes or answers an interactive confirmation. Previously, the only gate for this destructive path (managed-tree wipe + PATH change) was require_tty_or_yes plus an interactive/--yes confirm; existing scripts, CI jobs, or automation calling `numan setup nu path --yes or numan setup nu use <path> --yes` against a root with a managed install will now fail outright unless --force is also added.
Agent Prompt
## Issue description
`execute_use_path` and `execute_use_existing` (src/cmd/setup.rs) now require the new `--force` flag whenever a managed Nushell install exists, in addition to (not instead of) the previous `--yes`/confirm gate. Existing automation that passed `--yes` to `numan setup nu path` or `numan setup nu use <path>` to non-interactively replace a managed install will now hard-fail with 'Refusing ... while a managed Nushell install ... exists' since `--force` is a new, separate, mandatory flag.
## Issue Context
This change was introduced as part of a much larger unrelated diff bundled into a PR whose title/description only describes updating the `numan try` starter list. The new two-step force-gate (`--force` to enter the destructive path, then `--yes`/confirm for the actual wipe) is a deliberate safety hardening, but it changes the CLI contract for existing callers without being called out, versioned, or migration-documented anywhere in the PR description.
## Fix Focus Areas
- src/cmd/setup.rs[374-385]
- src/cmd/setup.rs[474-486]
- src/cmd/setup.rs[113-150] (NuSetupArgs constructors changed to require the new force parameter)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const STARTERS: &[StarterSpec] = &[ | ||
| // Primary: interactive fuzzy finder — a "wow" demo that shows Nu's structured data | ||
| StarterSpec { | ||
| id: "idanarye/nu_plugin_skim", | ||
| nu_minor: Some((0, 114)), | ||
| os: None, | ||
| }, | ||
| // Interactive TUI data explorer — visually impressive for newcomers | ||
| StarterSpec { | ||
| id: "abusch/nu_plugin_semver", | ||
| nu_minor: Some((0, 113)), | ||
| os: Some(Os::Windows), | ||
| id: "tonythethompson/nu_plugin_explore", | ||
| nu_minor: Some((0, 114)), | ||
| os: None, | ||
| }, | ||
| // Syntax highlighting — immediate visual payoff, works on any data | ||
| StarterSpec { | ||
| id: "cptpiepmatz/nu_plugin_highlight", | ||
| nu_minor: Some((0, 114)), | ||
| os: None, | ||
| }, |
There was a problem hiding this comment.
11. Unrelated large-scope changes in starter-list pr 🐞 Bug ⚙ Maintainability
The PR description states it only updates the curated numan try STARTERS list, but the actual diff bundles sweeping unrelated changes across doctor.rs, setup.rs, use_cmd.rs, bootstrap.rs, version_manager.rs, migrate_legacy.rs, migration_journal.rs, and several docs/CI files (new error types, a new migration-journal subsystem, a new mandatory --force CLI flag, and roadmap-contract tooling), none of which are mentioned in the PR title or description. This scope mismatch makes the change very difficult to review safely and increases the risk that unrelated, higher-risk behavioral changes (like the new mandatory --force gate) ship without adequate scrutiny under the guise of a small starter-list update.
Agent Prompt
## Issue description
The PR is described as a small, focused update to the `numan try` STARTERS list, but the actual diff includes large, unrelated feature work (migration journal subsystem, new mandatory `--force` setup flag, doctor repair changes, roadmap-contract tooling). This makes review, bisecting, and rollback much harder than necessary and hides higher-risk changes inside a low-risk-sounding PR.
## Issue Context
Per REVIEW.md's review checklist: 'Scope matches PR description; no unrelated refactors.' The bulk of the diff (doctor.rs, setup.rs, use_cmd.rs, bootstrap.rs, version_manager.rs, migrate_legacy.rs, migration_journal.rs, cross-repo-mirror/*, scripts/bump-contract.sh, scripts/check-roadmap-drift.py) has no relation to the starter-list change described in the PR title/description.
## Fix Focus Areas
- src/cmd/try_cmd.rs[38-74] (the actual in-scope change)
- src/cmd/setup.rs[1-600] (out-of-scope destructive-flow rework)
- src/state/migration_journal.rs (out-of-scope new subsystem)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer✅ Committed (4) · ☑ Fixed (4) Commits pushed directly to this PR — no separate fix PR opened. Process — 4 fixed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cmd/setup.rs (2)
1016-1033: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
run_with_path_snapshotraces with the sharedPathRestoreGuardmutex.This helper saves and restores
PATHwith no lock.src/util/test_paths.rsprovidesPathRestoreGuard, which serializes PATH access throughPATH_MUTEX, andsrc/nu/bootstrap.rsuses it inlatest_setup_short_circuit_ignores_dangling_active_marker.Unit tests in
src/cmd/setup.rsandsrc/nu/bootstrap.rscompile into the same test binary and run on parallel threads. A test holdingPathRestoreGuardand a test insiderun_with_path_snapshotcan interleave theirPATHreads and writes. The restore then writes back a value captured mid-mutation, which corruptsPATHfor every later test in that binary.Replace the ad-hoc snapshot with
PathRestoreGuardso both sites share one mutex.🔧 Proposed fix
fn run_with_path_snapshot<F, T>(body: F) -> T where F: FnOnce() -> T + std::panic::UnwindSafe, { - let saved = std::env::var("PATH").ok(); + let _guard = crate::util::test_paths::PathRestoreGuard::new(); let result = std::panic::catch_unwind(body); - match saved { - Some(p) => std::env::set_var("PATH", p), - None => std::env::remove_var("PATH"), - } match result { Ok(v) => v, Err(panic) => std::panic::resume_unwind(panic), } }
PathRestoreGuardrestoresPATHonDrop, including on the unwind path, so the explicit restore is no longer needed.🤖 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/cmd/setup.rs` around lines 1016 - 1033, Replace the manual PATH snapshot, catch_unwind, and restoration logic in run_with_path_snapshot with PathRestoreGuard, ensuring the guard is acquired before invoking the closure and remains in scope until it completes or unwinds. Remove the explicit environment restoration and use the guard’s Drop behavior to restore PATH while sharing PATH_MUTEX with other tests.
919-925: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRestore coverage for active-version marker clearing during managed removal.
The test that covered marker clearing in
remove_managed_nuwas deleted in this PR. That behavior did not go away — it changed. Line 574 now runs after the confirm and wraps the failure with context, and Line 549 clears a stale marker when no managed tree exists.Both are new, load-bearing branches with no remaining assertions.
remove_managed_nu_removes_directoryandremove_managed_nu_noop_when_absentonly check the directory.Add assertions that
read_active_version(root)isNoneafter each removal path, and a case where the confirm is declined and the marker survives.As per coding guidelines: "Add or update tests for behavior changes, including relevant failure paths."
🤖 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/cmd/setup.rs` around lines 919 - 925, Extend remove_managed_nu_removes_directory and remove_managed_nu_noop_when_absent to assert read_active_version(root) is None after removal, covering marker clearing for both existing and absent managed trees. Add a declined-confirmation test for remove_managed_nu that verifies the active-version marker remains present when removal is not approved.Source: Coding guidelines
AGENTS.md (1)
142-148: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument
setup_subcommand_lockand the new--forcegate.This PR adds two conventions that
AGENTS.mddoes not record:
setup_subcommand_lock(root, what, f)insrc/util/fs_safety.rsis now the single lock boundary for every destructive setup entry. Line 95 lists onlyOWNERSHIP_MARKER,acquire_mutation_lock, andassert_managed_file_owned. Line 142 describes the raw lock but not the setup-family wrapper or its(audit)line shape.numan setup nu pathandnuman setup nu use <path>now refuse to replace an existing managed tree without--force. Line 66 still readsnuman setup nu [VERSION]|remove|path|use <path>with no mention of the gate. This is user-visible behavior.Line 148 also lists
numan setup nu use <path>as the writer ofbinary_path, butexecute_use_pathnow writes it too.📝 Proposed additions
- fs_safety.rs — OWNERSHIP_MARKER, acquire_mutation_lock (advisory fd_lock mutex), assert_managed_file_owned (Phase 4) + fs_safety.rs — OWNERSHIP_MARKER, acquire_mutation_lock (advisory fd_lock mutex), setup_subcommand_lock (setup-family lock boundary + `(audit)` line), assert_managed_file_owned (Phase 4)- setup.rs — `numan setup nu [VERSION]|remove|path|use <path>` + `setup loader`: Nushell bootstrap + nushell-loader install + setup.rs — `numan setup nu [VERSION]|remove|path [--force]|use <path> [--force]` + `setup loader`: Nushell bootstrap + nushell-loader install; `--force` on `path`/`use` opts into deleting the managed treeAnd after Line 142:
+- **Setup mutation lock**: every destructive `numan setup nu` entry runs inside `setup_subcommand_lock(root, what, f)`; the helper acquires `acquire_mutation_lock`, emits `(audit) setup mutation lock acquired for {what} on '{root}'.`, and releases on Drop. Callers must not emit their own audit line.As per coding guidelines: "update
AGENTS.md,docs/, or command help when structure, conventions, or user-visible behavior changes."🤖 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 `@AGENTS.md` around lines 142 - 148, Update AGENTS.md to document setup_subcommand_lock as the shared lock boundary for destructive setup commands, including its audit-line format alongside the existing safety symbols. Document that numan setup nu path and numan setup nu use <path> require --force before replacing an existing managed tree, and update the setup command usage. Correct the active-version marker guidance to identify execute_use_path as a writer of binary_path.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 @.github/workflows/ci.yml:
- Around line 75-96: Fix the run block containing tagged_sha in
.github/workflows/ci.yml lines 75-96 by indenting the entire embedded Python
program and its closing quote within the run: | scalar. Apply the same
indentation fix to
cross-repo-mirror/numan-plugins/.github/workflows/roadmap-drift.yml lines 33-54
and cross-repo-mirror/numan-registry/.github/workflows/roadmap-drift.yml lines
33-54, preserving the script content and ensuring all three workflows remain
valid YAML.
In `@scripts/bump-contract.sh`:
- Around line 249-257: Update scripts/bump-contract.sh in the contract-document
generation block to use version-aware metadata for every contract-version field,
including procedure text and freeze-commit information, and add the generated
roadmap document to the frozen contract artifact set. In
docs/contracts/roadmap-v1.md lines 3-6, replace the unresolved freeze-commit
placeholder with accurate v1 metadata or remove the claim that the script
records it.
- Around line 153-155: Freeze contract content before deriving CONTENT_SHA:
update scripts/bump-contract.sh around CONTENT_SHA to require a clean committed
contract-content state, or create the immutable content commit before resolving
the SHA. Also update docs/contracts/roadmap-v1.md lines 110-117 to require
committing roadmap and drift-checker changes before running the bump command if
the script retains its current freeze model.
- Around line 296-299: Update scripts/bump-contract.sh lines 296-299 around
gh_auth_check to resolve the authenticated login and verify sufficient write or
maintain permission for tonythethompson/numan, tonythethompson/numan-plugins,
and tonythethompson/numan-registry before any file rewrite, commit, tag, or
push; fail early with an error if any check fails. Update
docs/contracts/roadmap-v1.md lines 141-143 to document the actual repository
owner and the permission checks performed by the script.
In `@src/cmd/doctor.rs`:
- Around line 2373-2405: Update doctor_fix_reconciles_migration_journal to
inspect the emitted events and assert a journal.migration_repaired record with
RepairStatus::Applied after the first execute_with_options call. Invoke
execute_with_options again on the same root, then assert the re-run emits no
additional journal.migration_repaired record while preserving the existing
filesystem and cleared-journal assertions.
In `@src/cmd/remove.rs`:
- Around line 19-21: Update the help text for the `yes` argument in the remove
command to state that `--yes` permits removal when stdin is not a terminal,
rather than claiming it skips confirmation prompts.
In `@src/cmd/setup.rs`:
- Line 543: Remove the duplicate require_tty_or_yes call in the managed Nushell
removal flow, specifically the later call around the block containing the first
gate near line 543 and the duplicate near line 560. Keep the existing comment
with the first call and preserve the single audit emission and refusal behavior.
- Around line 199-204: Remove PR, ticket, branch, and agent references from the
comments across src/cmd/setup.rs (199-204, 209-211, 443-444, and the identical
comment at 533), src/nu/bootstrap.rs (231-237, 717-720, and matching prefixes at
847 and 1035), src/util/fs_safety.rs (99-108), and src/util/confirm.rs (75-78
and the test comment at 131). Preserve only the non-obvious rationale:
mutation-lock coverage, the doctor repair entry point, active-version marker
persistence, legacy-binary avoidance, flow-aware gating, the single lock
boundary, and the injectable confirmation seam.
- Around line 614-617: Update the removal message in
remove_managed_nu_if_present so it no longer references the incorrect flow or
deprecated --use-existing flag; use a flow-neutral message stating that the
managed Nushell at managed_dir was removed, while preserving the existing
behavior in execute_use_path and execute_use_existing.
- Around line 364-385: In execute_use_path, move the normalized_version block
using NuVersion::from_binary and version_manager::normalize_version from before
the managed-directory force refusal to immediately before
snapshot_before_setup_mutation, matching execute_use_existing. Keep the force
gate and its existing refusal behavior unchanged, while preserving version
resolution before any destructive mutation.
- Around line 546-549: Update the clear_active_version call in the
!managed_dir.is_dir() branch of setup to attach anyhow context describing that
numan setup nu remove failed while clearing the active version for an absent
managed tree, matching the contextual handling used by the later call around
lines 574-579.
- Around line 198-247: Collapse execute_nu and execute_nu_impl onto a single
locking implementation, preserving the public entry point and direct-call
behavior through the shared setup_subcommand_lock path. Update the
execute_nu_impl documentation to state that the lock helper emits the audit
record unconditionally, removing the incorrect claim that direct callers must
provide their own audit prefix.
In `@src/cmd/try_cmd.rs`:
- Around line 45-50: Update the Explore StarterSpec entry and its related
fixtures/tests to use an exact registry package ID with a Nu 0.114-compatible
release; if no supported package exists, remove Explore from the starter
priority list and adjust the expected selection accordingly. Ensure
select_starter only receives registry-resolvable IDs.
In `@src/cmd/use_cmd.rs`:
- Around line 26-35: Ensure legacy migration mutations have a rollback snapshot
before execution: in src/cmd/use_cmd.rs lines 26-35, either create the required
activation snapshot before migrate_legacy_install or remove the migration so
list remains read-only; in src/cmd/doctor.rs lines 1451-1476, guard
migration_journal::reconcile with snapshot_ok and record "snapshot_unavailable"
when unavailable. Apply these changes to the list branch and
journal.migration_repaired block respectively.
- Around line 188-200: Update the Err branch in
refresh_cached_nu_paths_after_switch so remove_file failures are handled like
probe failures: emit a warning and return Ok instead of propagating the error.
Preserve the existing warning guidance to run CMD_INIT_REFRESH, ensuring
execute_switch remains successful after the active marker and switch have
already been applied.
In `@src/nu/bootstrap.rs`:
- Around line 1092-1130: The existing test only covers recovery to a valid
on-tree installation; add a sibling test for unresolved installation paths. Seed
an active version marker whose on-tree directory is absent and whose off-tree
binary is unreadable or missing, then call execute_nu_setup_with_installer and
assert it returns an error rather than succeeding with a non-existent path;
ensure the installer callback is handled consistently with the failure
expectation.
- Around line 721-728: In the surrounding function, compute a single
Option<String> for the normalized requested version near the start, preserving
the existing error context. Update the dest selection, the --yes short-circuit
marker, and the fresh-install marker/install flow to reuse this pinned value,
replacing their repeated normalize_version calls while keeping unpinned behavior
unchanged.
- Around line 257-260: Update the VERSION marker write in install_release to use
the normalized version value rather than the raw version argument, keeping the
marker content consistent with the per-version directory name while preserving
the existing path and compatibility behavior.
- Around line 745-793: Gate the existing-install short-circuit around the
resolved effective binary so it continues only when effective.is_file() is true.
Update the flow containing effective and the options.yes branch to fall through
to the normal install path when resolution returns a missing path, preventing
prepend_process_path, persist_user_path, the success message, and Ok(effective)
from running for nonexistent binaries.
- Around line 223-228: Update the ExtractConfig construction in the Nu archive
extraction flow to raise max_uncompressed_bytes above the measured uncompressed
size of official Nu release tarballs, with sufficient headroom for future
releases. Preserve the existing extraction behavior and context error handling
around archive_path.
In `@src/nu/paths.rs`:
- Around line 337-343: Update the VersionManagerError::ReadMarker branch to bind
its path field and include that marker path in the fail-closed anyhow error
message alongside the I/O source. Preserve the existing error behavior and
guidance, and leave the MalformedMarker branch unchanged.
In `@src/nu/version_manager.rs`:
- Around line 740-761: The managed binary layout is duplicated instead of using
the shared helpers. In src/nu/version_manager.rs lines 740-761, move
nu_binary_name and versioned_nu_dir above the mod tests block and update
legacy_managed_binary_with_bin to join versioned_nu_dir(root) with bin; in
src/state/migration_journal.rs lines 186-189, import nu_binary_name from
version_manager and reuse it at lines 187, 249, and 311 instead of repeating the
platform conditional.
- Around line 48-51: Collapse the multi-line #[error] literals for PathTraversal
and the other two affected enum variants into single rendered lines using
backslash continuations, removing the embedded newline and indentation while
preserving each message’s wording and formatting.
In `@src/state/migration_journal.rs`:
- Around line 362-415: Add tests alongside the existing PendingMigration
save/load tests covering both guard paths: write a journal with schema_version
set to SCHEMA_VERSION + 1 and assert PendingMigration::load returns Err, then
call PendingMigration::save with version "../evil", assert Err, and verify the
journal file was not created.
In `@src/util/confirm.rs`:
- Around line 71-83: Remove the duplicate public wrapper
require_tty_or_yes_with_tty and retain require_tty_or_yes_with_seam as the
single injectable TTY API. Move the existing seam documentation onto
require_tty_or_yes_with_seam, then update both bootstrap call sites to use that
name while preserving the existing behavior.
In `@src/util/fs_safety.rs`:
- Around line 99-141: Update the error context in setup_subcommand_lock so it
identifies the refused destructive operation without repeating the “another
Numan mutation is already in progress” message supplied by
acquire_mutation_lock. Preserve the existing root and operation details, and
leave the audit logging and lock lifetime unchanged.
In `@tests/setup_nu_test.rs`:
- Around line 62-63: Update the comments near the injected installer and the
referenced locations to remove PR-specific identifiers and review context,
retaining only concise explanations of the non-obvious behavioral reason for the
fixture or test.
- Around line 371-374: Update both force-gate tests around stage_fake_nu and
execute_nu to use version_manager::version_binary(root, version) for the managed
fixture, matching the current <root>/tools/nushell/<version>/<bin> layout. Write
the active-version marker before invoking execute_nu, and assert against the
versioned binary after both refusal and forced replacement paths.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 142-148: Update AGENTS.md to document setup_subcommand_lock as the
shared lock boundary for destructive setup commands, including its audit-line
format alongside the existing safety symbols. Document that numan setup nu path
and numan setup nu use <path> require --force before replacing an existing
managed tree, and update the setup command usage. Correct the active-version
marker guidance to identify execute_use_path as a writer of binary_path.
In `@src/cmd/setup.rs`:
- Around line 1016-1033: Replace the manual PATH snapshot, catch_unwind, and
restoration logic in run_with_path_snapshot with PathRestoreGuard, ensuring the
guard is acquired before invoking the closure and remains in scope until it
completes or unwinds. Remove the explicit environment restoration and use the
guard’s Drop behavior to restore PATH while sharing PATH_MUTEX with other tests.
- Around line 919-925: Extend remove_managed_nu_removes_directory and
remove_managed_nu_noop_when_absent to assert read_active_version(root) is None
after removal, covering marker clearing for both existing and absent managed
trees. Add a declined-confirmation test for remove_managed_nu that verifies the
active-version marker remains present when removal is not approved.
🪄 Autofix
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: 7bfc7d5b-9975-4ec8-8ca1-1cb21f417d42
📒 Files selected for processing (34)
.github/workflows/ci.ymlAGENTS.mdcross-repo-mirror/README.mdcross-repo-mirror/numan-plugins/.github/workflows/roadmap-drift.ymlcross-repo-mirror/numan-plugins/docs/roadmap.mdcross-repo-mirror/numan-registry/.github/workflows/roadmap-drift.ymlcross-repo-mirror/numan-registry/docs/roadmap.mdcross-repo-mirror/snapshot-tests/mirror_dry_run.shdocs/contracts/roadmap-v1.mddocs/plans/consolidated-multi-repo-roadmap.mdscripts/bump-contract.shscripts/check-roadmap-drift.pysrc/cmd/doctor.rssrc/cmd/remove.rssrc/cmd/setup.rssrc/cmd/snapshot.rssrc/cmd/try_cmd.rssrc/cmd/use_cmd.rssrc/lib.rssrc/nu/bootstrap.rssrc/nu/migrate_legacy.rssrc/nu/mod.rssrc/nu/paths.rssrc/nu/version_manager.rssrc/state/migration_journal.rssrc/state/mod.rssrc/util/confirm.rssrc/util/fs_safety.rssrc/util/hints.rssrc/util/mod.rssrc/util/test_paths.rstests/doctor_test.rstests/setup_nu_test.rstests/support/acceptance/model.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (17)
**/*
📄 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:
cross-repo-mirror/numan-plugins/docs/roadmap.mdcross-repo-mirror/numan-registry/docs/roadmap.mdsrc/nu/mod.rscross-repo-mirror/snapshot-tests/mirror_dry_run.shsrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rsscripts/check-roadmap-drift.pydocs/plans/consolidated-multi-repo-roadmap.mdtests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rsAGENTS.mdcross-repo-mirror/README.mdsrc/cmd/snapshot.rssrc/cmd/try_cmd.rsdocs/contracts/roadmap-v1.mdtests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rsscripts/bump-contract.shsrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.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:
cross-repo-mirror/numan-plugins/docs/roadmap.mdcross-repo-mirror/numan-registry/docs/roadmap.mdsrc/nu/mod.rscross-repo-mirror/snapshot-tests/mirror_dry_run.shsrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rsscripts/check-roadmap-drift.py.github/workflows/ci.ymldocs/plans/consolidated-multi-repo-roadmap.mdcross-repo-mirror/numan-registry/.github/workflows/roadmap-drift.ymltests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rsAGENTS.mdcross-repo-mirror/README.mdsrc/cmd/snapshot.rssrc/cmd/try_cmd.rsdocs/contracts/roadmap-v1.mdcross-repo-mirror/numan-plugins/.github/workflows/roadmap-drift.ymltests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rsscripts/bump-contract.shsrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.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:
cross-repo-mirror/numan-plugins/docs/roadmap.mdcross-repo-mirror/numan-registry/docs/roadmap.mdsrc/nu/mod.rssrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rsdocs/plans/consolidated-multi-repo-roadmap.mdtests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rsAGENTS.mdcross-repo-mirror/README.mdsrc/cmd/snapshot.rssrc/cmd/try_cmd.rsdocs/contracts/roadmap-v1.mdtests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rssrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.rs
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's established serialization and module conventions rather than introducing unrelated refactors.
Files:
cross-repo-mirror/numan-plugins/docs/roadmap.mdcross-repo-mirror/numan-registry/docs/roadmap.mdsrc/nu/mod.rssrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rsdocs/plans/consolidated-multi-repo-roadmap.mdtests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rsAGENTS.mdcross-repo-mirror/README.mdsrc/cmd/snapshot.rssrc/cmd/try_cmd.rsdocs/contracts/roadmap-v1.mdtests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rssrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.rs
**/*.md
📄 CodeRabbit inference engine (REVIEW.md)
Update documentation and
AGENTS.mdwhen project structure or conventions change.
Files:
cross-repo-mirror/numan-plugins/docs/roadmap.mdcross-repo-mirror/numan-registry/docs/roadmap.mddocs/plans/consolidated-multi-repo-roadmap.mdAGENTS.mdcross-repo-mirror/README.mddocs/contracts/roadmap-v1.md
**/*.{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/nu/mod.rssrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rsscripts/check-roadmap-drift.pytests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rssrc/cmd/snapshot.rssrc/cmd/try_cmd.rstests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rssrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.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 usewrite_json_atomic; partial writes are not allowed.
Pending activation, autoload, and lifecycle journals must be stored under$NUMAN_ROOT/state/.
Module autoload identity must match all four fields: Nu executable hash, Nu version, vendor autoload directory, and managed file path; the lockfilemodule_activationvalue is authoritative.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass paths to Nu only throu...
Files:
src/nu/mod.rssrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rstests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rssrc/cmd/snapshot.rssrc/cmd/try_cmd.rstests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rssrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.Code must pass
cargo clippy -- -D warningsandcargo fmt --check; do not broaden Clippy scope with--all-targetswhen matching CI.
Files:
src/nu/mod.rssrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rstests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rssrc/cmd/snapshot.rssrc/cmd/try_cmd.rstests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rssrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.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/nu/mod.rssrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rstests/support/acceptance/model.rstests/doctor_test.rssrc/util/test_paths.rssrc/util/fs_safety.rssrc/cmd/snapshot.rssrc/cmd/try_cmd.rstests/setup_nu_test.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rssrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.rs
src/nu/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
The active Nu version is authoritative only from
nu_state/active-version.json; write the marker through managed setup/use operations and preserve optional off-treebinary_pathmetadata.
Files:
src/nu/mod.rssrc/nu/migrate_legacy.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use
serde/serde_jsonfor JSON state andtomlfor configuration; preserve backward-compatible serialized forms where fields are optional and defaulted.
Files:
src/nu/mod.rssrc/state/mod.rssrc/lib.rssrc/util/hints.rssrc/util/mod.rssrc/util/test_paths.rssrc/util/fs_safety.rssrc/cmd/snapshot.rssrc/cmd/try_cmd.rssrc/nu/migrate_legacy.rssrc/cmd/remove.rssrc/util/confirm.rssrc/cmd/use_cmd.rssrc/cmd/doctor.rssrc/state/migration_journal.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/nu/version_manager.rssrc/cmd/setup.rs
src/state/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/state/**/*.rs: Write JSON state files atomically withwrite_json_atomicusing a temporary file in the same directory followed by persist.
Acquireacquire_mutation_lock(root)before mutations; the second acquisition for the same root must fail immediately, and the RAII guard must remain held through the mutation.
Files:
src/state/mod.rssrc/state/migration_journal.rs
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
**/*.{cs,py}: When modifying managed host or containerized readiness components, never callStopTrackedHost,RestartAsync, or equivalent host-kill methods unless active request leases are confirmed to be zero. Busy hosts reportingbusy_reasonoractive_requests > 0must not be restarted, and a ready/available provider must not be downgraded solely because a background probe refresh began.
ForSessionWorkflowStage, preserve strictly ascending order:Foundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum members rather than raw integer literals. New or renumbered members require a legacy-compatible JSON converter, and reordered enums require checking every solution-wide inequality comparison for preserved semantic meaning.
Files:
scripts/check-roadmap-drift.py
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use inline unit tests and integration tests under
tests/; inject fake platforms, registrars, unregistrars, and candidate runners rather than relying on external Nu processes in unit tests.
Files:
tests/support/acceptance/model.rstests/doctor_test.rstests/setup_nu_test.rs
src/cmd/{install,update,remove,activate,deactivate,init,doctor,nupm}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Create an activation snapshot before install, update, remove, activate, deactivate,
init --refresh, nupm import, and doctor repair mutations.
Files:
src/cmd/remove.rssrc/cmd/doctor.rs
src/state/{journal,plugin_deactivate_journal,migration_journal,autoload_journal,lifecycle_journal}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Journal multi-step mutations durably and atomically, advancing each stage before dependent state updates and reconciling pending journals on subsequent commands.
Files:
src/state/migration_journal.rs
src/cmd/{activate,deactivate,setup,init}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Pass executable paths and plugin metadata through environment variables (
NUMAN_PLUGIN_BINARY,NUMAN_PLUGIN_CONFIG,NUMAN_PLUGIN_NAME); keep the Nu program string a compile-time constant with no runtime interpolation.
Files:
src/cmd/setup.rs
🪛 actionlint (1.7.12)
.github/workflows/ci.yml
[error] 81-81: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 LanguageTool
cross-repo-mirror/README.md
[style] ~35-~35: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... at the start of the job, then runs the exact same scripts/check-roadmap-drift.py agains...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
docs/contracts/roadmap-v1.md
[uncategorized] ~176-~176: The official name of this software platform is spelled with a capital “H”.
Context: ...rtifacts for the two sibling repos. * [.github/workflows/ci.yml](../../.github/workflo...
(GITHUB)
[uncategorized] ~176-~176: The official name of this software platform is spelled with a capital “H”.
Context: ...bling repos. * .github/workflows/ci.yml — the roadmap-drift...
(GITHUB)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 82-82: syntax error: could not find expected ':'
(syntax)
cross-repo-mirror/numan-registry/.github/workflows/roadmap-drift.yml
[error] 40-40: syntax error: could not find expected ':'
(syntax)
cross-repo-mirror/numan-plugins/.github/workflows/roadmap-drift.yml
[error] 40-40: syntax error: could not find expected ':'
(syntax)
🔍 Remote MCP GitHub Copilot
Relevant review context
- The PR adds
tonythethompson/nu_plugin_exploretoSTARTERS, but the upstream repository isamtoine/nu_plugin_explore; thetonythethompsonowner does not appear in repository search. This prevents the curated entry from matching the official registry package ID. - The upstream Explore repository is archived, targets Nushell
0.102.0, and documents the sameopen Cargo.toml | nu_plugin_exploreusage hint. Its maintenance/compatibility should be confirmed before treating it as a Nu 0.114 starter. - The current official registry catalog lists 44 packages and includes
cptpiepmatz/nu_plugin_highlightwith Nu 0.114 and 0.113 releases, but does not list Explore. Thus Highlight can serve the fallback, while Explore currently cannot be selected from the catalog. - Resolver behavior checks all package versions, not only the latest, so Highlight’s older 0.113 release can satisfy the universal fallback on Nu 0.113.
- PR
#111remains open; Rust analysis and Greptile checks were still in progress, with CodeQL neutral and CodeFactor successful. No review threads were present.
🔇 Additional comments (54)
src/cmd/remove.rs (2)
3-3: LGTM!Also applies to: 23-38
265-315: 📐 Maintainability & Code QualityHave CI report the required Rust gate results before merge.
This sandbox failed during dependency build due to missing OpenSSL/
pkg-config/compiler tooling, so the available result does not showcargo test,cargo clippy -- -D warnings, andcargo fmt --checkcompletion.src/cmd/snapshot.rs (1)
3-3: LGTM!Also applies to: 205-241, 265-327
docs/plans/consolidated-multi-repo-roadmap.md (1)
304-304: LGTM!Also applies to: 319-321
cross-repo-mirror/README.md (1)
7-8: LGTM!Also applies to: 17-20, 34-83, 93-102
cross-repo-mirror/numan-plugins/docs/roadmap.md (1)
9-14: LGTM!cross-repo-mirror/numan-registry/docs/roadmap.md (1)
9-14: LGTM!scripts/check-roadmap-drift.py (1)
155-171: LGTM!cross-repo-mirror/snapshot-tests/mirror_dry_run.sh (1)
30-33: LGTM!src/cmd/try_cmd.rs (1)
39-44: LGTM!Also applies to: 51-73, 234-234, 282-288, 413-413, 437-450
src/util/mod.rs (1)
6-7: LGTM!src/util/test_paths.rs (1)
1-64: LGTM!tests/doctor_test.rs (1)
39-40: LGTM!Also applies to: 62-66, 102-106
tests/setup_nu_test.rs (1)
13-13: LGTM!Also applies to: 40-51, 77-84, 88-96, 134-187, 230-251, 287-299, 334-350
tests/support/acceptance/model.rs (1)
101-105: LGTM!src/state/migration_journal.rs (4)
93-117: LGTM!Also applies to: 119-179
236-336: LGTM!
127-151: LGTM!
216-234: LGTM!src/nu/migrate_legacy.rs (4)
55-126: LGTM!
181-227: LGTM!Also applies to: 340-358
360-784: LGTM!
134-136: 🩺 Stability & AvailabilityNo change needed for legacy-migration callers.
Current code does not call
migrate_legacy_installfrom an unlocked path.src/nu/mod.rs (1)
3-7: LGTM!src/nu/version_manager.rs (5)
110-126: LGTM!Also applies to: 152-198, 206-216
224-240: LGTM!
319-406: LGTM!
410-446: LGTM!
505-552: LGTM!src/nu/paths.rs (3)
382-389: LGTM!Also applies to: 395-403
483-501: LGTM!
611-625: LGTM!Also applies to: 845-878, 902-945
src/cmd/use_cmd.rs (3)
84-123: LGTM!
126-162: LGTM!
217-227: LGTM!Also applies to: 305-449, 463-482
src/cmd/doctor.rs (4)
650-683: LGTM!
1528-1529: LGTM!
31-39: LGTM!Also applies to: 131-131, 1203-1203
1628-1628: LGTM!Also applies to: 2113-2113, 2328-2371, 2407-2456
src/state/mod.rs (1)
8-8: LGTM!src/lib.rs (1)
1-6: LGTM!src/nu/bootstrap.rs (3)
329-329: LGTM!Also applies to: 477-478
465-465: LGTM!Also applies to: 533-537
879-882: LGTM!src/cmd/setup.rs (6)
11-15: LGTM!
113-159: LGTM!
216-238: LGTM!
259-289: LGTM!
1005-1009: LGTM!Also applies to: 1050-1050, 1077-1077, 1111-1111
78-95: 🩺 Stability & AvailabilityNo change needed for the
--forcearguments.
actionis a subcommand, so the top-level--forceand subcommand--forceflags live in separate clap scopes and avoid a duplicate ID parsing failure.src/util/fs_safety.rs (1)
441-483: LGTM!src/util/confirm.rs (1)
133-162: LGTM!AGENTS.md (1)
83-92: LGTM!src/util/hints.rs (1)
144-149: 📐 Maintainability & Code QualityNo change needed.
CMD_USEis imported intosrc/cmd/doctor.rs, used for the doctor fix hint, and covered by a test assertion.
| tagged_sha=$(curl --fail-with-body --silent --show-error \ | ||
| -H "Authorization: Bearer ${GITHUB_TOKEN}" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "User-Agent: numan-ci" \ | ||
| "https://api.github.com/repos/tonythethompson/numan/git/refs/tags/${CONTRACT_TAG}" \ | ||
| | python -c " | ||
| import json, os, sys, urllib.request | ||
| ref = json.load(sys.stdin) | ||
| sha = ref['object']['sha'] | ||
| if ref['object']['type'] == 'tag': | ||
| headers = { | ||
| 'Accept': 'application/vnd.github+json', | ||
| 'User-Agent': 'numan-ci', | ||
| } | ||
| token = os.environ.get('GITHUB_TOKEN', '') | ||
| if token: | ||
| headers['Authorization'] = f'Bearer {token}' | ||
| req = urllib.request.Request(ref['object']['url'], headers=headers) | ||
| tag = json.load(urllib.request.urlopen(req)) | ||
| sha = tag['object']['sha'] | ||
| print(sha) | ||
| ") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fix the YAML block-scalar indentation in all roadmap workflows.
The Python source starts at YAML column zero. This ends the run: | scalar and makes each workflow invalid YAML. None of the contract validation jobs can load.
.github/workflows/ci.yml#L75-L96: indent the embedded Python program and its closing quote within therun: |scalar.cross-repo-mirror/numan-plugins/.github/workflows/roadmap-drift.yml#L33-L54: indent the embedded Python program and its closing quote within therun: |scalar.cross-repo-mirror/numan-registry/.github/workflows/roadmap-drift.yml#L33-L54: indent the embedded Python program and its closing quote within therun: |scalar.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 81-81: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 YAMLlint (1.37.1)
[error] 82-82: syntax error: could not find expected ':'
(syntax)
📍 Affects 3 files
.github/workflows/ci.yml#L75-L96(this comment)cross-repo-mirror/numan-plugins/.github/workflows/roadmap-drift.yml#L33-L54cross-repo-mirror/numan-registry/.github/workflows/roadmap-drift.yml#L33-L54
🤖 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 @.github/workflows/ci.yml around lines 75 - 96, Fix the run block containing
tagged_sha in .github/workflows/ci.yml lines 75-96 by indenting the entire
embedded Python program and its closing quote within the run: | scalar. Apply
the same indentation fix to
cross-repo-mirror/numan-plugins/.github/workflows/roadmap-drift.yml lines 33-54
and cross-repo-mirror/numan-registry/.github/workflows/roadmap-drift.yml lines
33-54, preserving the script content and ensuring all three workflows remain
valid YAML.
Source: Linters/SAST tools
| # --- 2. Freeze content SHA (roadmap + drift script already on tip) ------- | ||
| CONTENT_SHA="$(git rev-parse HEAD)" | ||
| log "content SHA (freeze target) = $CONTENT_SHA" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Freeze the edited contract content before deriving CONTENT_SHA.
The documented procedure permits uncommitted roadmap edits. Line 154 then resolves the old HEAD, while Lines 263-266 do not stage the roadmap or drift checker. The tag and workflow pins can therefore reference old content after a successful local drift check.
scripts/bump-contract.sh#L153-L155: require a clean, committed contract-content state before resolvingCONTENT_SHA, or create the immutable content commit before deriving the SHA.docs/contracts/roadmap-v1.md#L110-L117: require the author to commit the roadmap and drift-checker changes before running the bump command if the script keeps its current freeze model.
📍 Affects 2 files
scripts/bump-contract.sh#L153-L155(this comment)docs/contracts/roadmap-v1.md#L110-L117
🤖 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 `@scripts/bump-contract.sh` around lines 153 - 155, Freeze contract content
before deriving CONTENT_SHA: update scripts/bump-contract.sh around CONTENT_SHA
to require a clean committed contract-content state, or create the immutable
content commit before resolving the SHA. Also update
docs/contracts/roadmap-v1.md lines 110-117 to require committing roadmap and
drift-checker changes before running the bump command if the script retains its
current freeze model.
| # --- 4. Refresh contract doc, then commit pins before tagging ----------- | ||
| if [ -f "$CONTRACT_DOC_DIR/roadmap-v${VERSION_LABEL}.md" ]; then | ||
| log "contract doc already exists at docs/contracts/roadmap-v${VERSION_LABEL}.md — leaving as-is" | ||
| else | ||
| log "creating contract doc from v1 template" | ||
| sed "s/^v1$/v${VERSION_LABEL}/g; s/Roadmap Contract v1/Roadmap Contract v${VERSION_LABEL}/g" \ | ||
| "$CONTRACT_DOC_DIR/roadmap-v1.md" \ | ||
| > "$CONTRACT_DOC_DIR/roadmap-v${VERSION_LABEL}.md" | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Generate complete contract metadata for every version.
The sed command changes only the title. A generated roadmap-v2.md retains the v1 tag, v1 procedure text, and the unresolved freeze-commit placeholder. It cannot describe the immutable version that the tag represents.
scripts/bump-contract.sh#L249-L257: use a versioned template that sets every contract-version field and include the generated document in the frozen contract artifact set.docs/contracts/roadmap-v1.md#L3-L6: replace the unresolved freeze-commit placeholder with accurate v1 metadata, or remove the claim that the script records it.
📍 Affects 2 files
scripts/bump-contract.sh#L249-L257(this comment)docs/contracts/roadmap-v1.md#L3-L6
🤖 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 `@scripts/bump-contract.sh` around lines 249 - 257, Update
scripts/bump-contract.sh in the contract-document generation block to use
version-aware metadata for every contract-version field, including procedure
text and freeze-commit information, and add the generated roadmap document to
the frozen contract artifact set. In docs/contracts/roadmap-v1.md lines 3-6,
replace the unresolved freeze-commit placeholder with accurate v1 metadata or
remove the claim that the script records it.
| gh_auth_check() { | ||
| gh auth status >/dev/null 2>&1 || { err "gh not authenticated"; exit 5; } | ||
| } | ||
| gh_auth_check |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Check repository permissions before any contract mutation.
gh auth status confirms only that a user is authenticated. It does not confirm write or maintain permission for tonythethompson/numan, tonythethompson/numan-plugins, and tonythethompson/numan-registry. A sibling permission failure can occur after the source branch is pushed and leave the coordinated operation incomplete.
scripts/bump-contract.sh#L296-L299: resolve the authenticated login and verify sufficient permission on all three repositories before rewriting files, committing, tagging, or pushing.docs/contracts/roadmap-v1.md#L141-L143: document the actual repository owner and the permission check that the script performs.
📍 Affects 2 files
scripts/bump-contract.sh#L296-L299(this comment)docs/contracts/roadmap-v1.md#L141-L143
🤖 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 `@scripts/bump-contract.sh` around lines 296 - 299, Update
scripts/bump-contract.sh lines 296-299 around gh_auth_check to resolve the
authenticated login and verify sufficient write or maintain permission for
tonythethompson/numan, tonythethompson/numan-plugins, and
tonythethompson/numan-registry before any file rewrite, commit, tag, or push;
fail early with an error if any check fails. Update docs/contracts/roadmap-v1.md
lines 141-143 to document the actual repository owner and the permission checks
performed by the script.
| #[test] | ||
| fn doctor_fix_reconciles_migration_journal() { | ||
| let dir = TempDir::new().unwrap(); | ||
| let root = dir.path(); | ||
| // Same pre-stage as `doctor_reports_migration_journal_finding`. | ||
| let tools = root.join("tools").join("nushell"); | ||
| std::fs::create_dir_all(&tools).unwrap(); | ||
| std::fs::create_dir_all(tools.join("0.113.1")).unwrap(); | ||
| PendingMigration { | ||
| schema_version: crate::state::migration_journal::SCHEMA_VERSION, | ||
| version: "0.113.1".to_string(), | ||
| stage: crate::state::migration_journal::MigrationStage::Prepared, | ||
| } | ||
| .save(root) | ||
| .unwrap(); | ||
|
|
||
| let args = DoctorArgs { | ||
| scan: false, | ||
| json: false, | ||
| nupm_home: None, | ||
| }; | ||
| let _ = execute_with_options(&args, root, test_doctor_options()).unwrap(); | ||
|
|
||
| // After `doctor --fix`: the empty subdir AND the journal must be gone. | ||
| assert!( | ||
| !tools.join("0.113.1").exists(), | ||
| "empty versioned subdir must be removed by reconcile" | ||
| ); | ||
| assert!( | ||
| PendingMigration::load(root).unwrap().is_none(), | ||
| "journal must be cleared by reconcile" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the repair record, and add a re-run case.
This test checks only the filesystem effects. It does not assert that a journal.migration_repaired record was produced with RepairStatus::Applied. The production comment at Line 1449 states the goal is to keep the Applied record honest, so the record is the contract worth pinning.
Add a second execute_with_options call on the same root and assert the re-run produces no second journal.migration_repaired record. That case exercises the already-cleared branch flagged at Line 1455.
As per coding guidelines: "Tests must cover failure modes, not only successful execution."
🤖 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/cmd/doctor.rs` around lines 2373 - 2405, Update
doctor_fix_reconciles_migration_journal to inspect the emitted events and assert
a journal.migration_repaired record with RepairStatus::Applied after the first
execute_with_options call. Invoke execute_with_options again on the same root,
then assert the re-run emits no additional journal.migration_repaired record
while preserving the existing filesystem and cleared-journal assertions.
Source: Coding guidelines
| // ── save / load / delete ──────────────────────────────────────────────── | ||
|
|
||
| #[test] | ||
| fn roundtrip_save_load() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| let root = tmp.path(); | ||
|
|
||
| let j = PendingMigration { | ||
| schema_version: SCHEMA_VERSION, | ||
| version: "0.113.1".to_string(), | ||
| stage: MigrationStage::Prepared, | ||
| }; | ||
| j.save(root).unwrap(); | ||
| let loaded = PendingMigration::load(root).unwrap().unwrap(); | ||
| assert_eq!(loaded.schema_version, SCHEMA_VERSION); | ||
| assert_eq!(loaded.version, "0.113.1"); | ||
| assert_eq!(loaded.stage, MigrationStage::Prepared); | ||
| } | ||
|
|
||
| #[test] | ||
| fn load_returns_none_when_absent() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| assert!(PendingMigration::load(tmp.path()).unwrap().is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn delete_removes_file() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| write_journal(tmp.path(), "0.113.1", MigrationStage::Prepared); | ||
| assert!(PendingMigration::load(tmp.path()).unwrap().is_some()); | ||
| PendingMigration::delete(tmp.path()).unwrap(); | ||
| assert!(PendingMigration::load(tmp.path()).unwrap().is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn delete_is_idempotent() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| PendingMigration::delete(tmp.path()).unwrap(); | ||
| } | ||
|
|
||
| // ── stage serde roundtrip ─────────────────────────────────────────────── | ||
|
|
||
| #[test] | ||
| fn stage_serde_roundtrip() { | ||
| for stage in [ | ||
| MigrationStage::Prepared, | ||
| MigrationStage::Renamed, | ||
| MigrationStage::Active, | ||
| ] { | ||
| let s = serde_json::to_string(&stage).unwrap(); | ||
| let parsed: MigrationStage = serde_json::from_str(&s).unwrap(); | ||
| assert_eq!(parsed, stage); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add tests for the two new guard paths.
The suite does not exercise load schema-version rejection or save unsafe-version rejection. Both guards are new in this change, and the repository requires tests to cover failure modes. Add one test that writes a journal with schema_version: SCHEMA_VERSION + 1 and asserts load returns Err. Add one test that calls save with version: "../evil" and asserts Err plus no file on disk.
As per coding guidelines: "Tests must cover failure modes, not only successful execution."
🤖 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/state/migration_journal.rs` around lines 362 - 415, Add tests alongside
the existing PendingMigration save/load tests covering both guard paths: write a
journal with schema_version set to SCHEMA_VERSION + 1 and assert
PendingMigration::load returns Err, then call PendingMigration::save with
version "../evil", assert Err, and verify the journal file was not created.
Source: Coding guidelines
| /// Same as [`require_tty_or_yes`] but lets the caller inject the TTY | ||
| /// decision so unit tests can cover all three branches (explicit `--yes`, | ||
| /// TTY-yes, TTY-no) without spawning a real TTY. | ||
| /// | ||
| /// This addresses PR #69's WDr (in `src/util/confirm.rs`): the non-TTY bail | ||
| /// branch was previously unreachable from a unit test because | ||
| /// `std::io::stdin().is_terminal()` was not injectable. CI now exercises | ||
| /// every branch via this seam. | ||
| pub fn require_tty_or_yes_with_tty(yes: bool, what: &str, is_tty: bool) -> Result<()> { | ||
| require_tty_or_yes_with_seam(yes, what, is_tty) | ||
| } | ||
|
|
||
| pub fn require_tty_or_yes_with_seam(yes: bool, what: &str, is_tty: bool) -> Result<()> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two public functions now do exactly the same thing.
require_tty_or_yes_with_tty and require_tty_or_yes_with_seam have the same signature and the same behavior. The first is a one-line pass-through to the second.
The codebase is already split across both names:
src/nu/bootstrap.rsLines 497 and 819 callrequire_tty_or_yes_with_tty.src/cmd/snapshot.rsLine 212 andsrc/cmd/remove.rsLine 37 callrequire_tty_or_yes_with_seam.
The doc block explaining the test seam sits on _with_tty, while _with_seam — the name the newer call sites use and the one the tests at Lines 138-155 exercise — carries no doc at all.
Keep one name. Move the doc onto it, and update the call sites.
♻️ Proposed refactor
-/// Same as [`require_tty_or_yes`] but lets the caller inject the TTY
-/// decision so unit tests can cover all three branches (explicit `--yes`,
-/// TTY-yes, TTY-no) without spawning a real TTY.
-///
-/// This addresses PR `#69`'s WDr (in `src/util/confirm.rs`): the non-TTY bail
-/// branch was previously unreachable from a unit test because
-/// `std::io::stdin().is_terminal()` was not injectable. CI now exercises
-/// every branch via this seam.
-pub fn require_tty_or_yes_with_tty(yes: bool, what: &str, is_tty: bool) -> Result<()> {
- require_tty_or_yes_with_seam(yes, what, is_tty)
-}
-
+/// Same as [`require_tty_or_yes`] but lets the caller inject the TTY
+/// decision, so unit tests can cover all three branches (explicit `--yes`,
+/// TTY without `--yes`, non-TTY without `--yes`) without a real TTY.
pub fn require_tty_or_yes_with_seam(yes: bool, what: &str, is_tty: bool) -> Result<()> {Then change the two require_tty_or_yes_with_tty call sites in src/nu/bootstrap.rs to require_tty_or_yes_with_seam.
🤖 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/util/confirm.rs` around lines 71 - 83, Remove the duplicate public
wrapper require_tty_or_yes_with_tty and retain require_tty_or_yes_with_seam as
the single injectable TTY API. Move the existing seam documentation onto
require_tty_or_yes_with_seam, then update both bootstrap call sites to use that
name while preserving the existing behavior.
| /// Acquire the root mutation lock, run a destructive setup subcommand, and | ||
| /// release the lock on return. | ||
| /// | ||
| /// Every destructive setup entry point (install, off-path registration, | ||
| /// PATH-Nu registration, managed removal, derive/active/upgrade via | ||
| /// `numan use`) flows through this helper so the lock boundary has exactly | ||
| /// one source of truth — closing PR #69's WCr (`setup_family_mutation_lock`) | ||
| /// and ensuring that a concurrent `numan use`, `numan install`, or | ||
| /// `numan doctor --fix` cannot interleave filesystem mutations on the same | ||
| /// root. | ||
| /// | ||
| /// `what` is a short human-readable label (e.g. `"Nushell install"`, | ||
| /// `"off-path Nu registration"`, `"managed Nushell removal"`); it lands | ||
| /// in the audit log alongside the `(audit)` prefix so safe-batch automation | ||
| /// can grep one consistent shape across the whole destructive setup | ||
| /// surface: | ||
| /// | ||
| /// ```text | ||
| /// (audit) setup mutation lock acquired for {what} on '{root}'. | ||
| /// ``` | ||
| /// | ||
| /// The closure runs while the lock is held; returns the closure's | ||
| /// `Result<T>` verbatim. The lock is released on Drop at the end of this | ||
| /// function's scope (also on panic — RAII). | ||
| /// | ||
| /// Use [`crate::util::confirm::require_tty_or_yes`] for non-TTY / `--yes` | ||
| /// gating; this helper does **not** add TTY checks. | ||
| pub fn setup_subcommand_lock<T, F>(root: &Path, what: &str, f: F) -> Result<T> | ||
| where | ||
| F: FnOnce() -> Result<T>, | ||
| { | ||
| let _lock = acquire_mutation_lock(root).with_context(|| { | ||
| format!( | ||
| "Refusing destructive '{what}' on '{}': another Numan mutation is already in progress.", | ||
| root.display() | ||
| ) | ||
| })?; | ||
| eprintln!( | ||
| "(audit) setup mutation lock acquired for {what} on '{}'.", | ||
| root.display() | ||
| ); | ||
| f() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Lock lifetime and ordering are correct.
let _lock binds the guard for the full function scope, so it survives f(). A let _ would have dropped it immediately. The audit line fires after acquisition, and try_write is non-blocking, so there is no hang risk.
One note: the .with_context string repeats the phrase already present in acquire_mutation_lock's error at Line 88. Rendering with {:#} prints "another Numan mutation is already in progress" twice. Consider making the context state the refused operation only.
♻️ Optional tidy-up
let _lock = acquire_mutation_lock(root).with_context(|| {
- format!(
- "Refusing destructive '{what}' on '{}': another Numan mutation is already in progress.",
- root.display()
- )
+ format!("Refusing destructive '{what}' on '{}'", root.display())
})?;The test at Line 480 greps "another Numan mutation", which the inner error still supplies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Acquire the root mutation lock, run a destructive setup subcommand, and | |
| /// release the lock on return. | |
| /// | |
| /// Every destructive setup entry point (install, off-path registration, | |
| /// PATH-Nu registration, managed removal, derive/active/upgrade via | |
| /// `numan use`) flows through this helper so the lock boundary has exactly | |
| /// one source of truth — closing PR #69's WCr (`setup_family_mutation_lock`) | |
| /// and ensuring that a concurrent `numan use`, `numan install`, or | |
| /// `numan doctor --fix` cannot interleave filesystem mutations on the same | |
| /// root. | |
| /// | |
| /// `what` is a short human-readable label (e.g. `"Nushell install"`, | |
| /// `"off-path Nu registration"`, `"managed Nushell removal"`); it lands | |
| /// in the audit log alongside the `(audit)` prefix so safe-batch automation | |
| /// can grep one consistent shape across the whole destructive setup | |
| /// surface: | |
| /// | |
| /// ```text | |
| /// (audit) setup mutation lock acquired for {what} on '{root}'. | |
| /// ``` | |
| /// | |
| /// The closure runs while the lock is held; returns the closure's | |
| /// `Result<T>` verbatim. The lock is released on Drop at the end of this | |
| /// function's scope (also on panic — RAII). | |
| /// | |
| /// Use [`crate::util::confirm::require_tty_or_yes`] for non-TTY / `--yes` | |
| /// gating; this helper does **not** add TTY checks. | |
| pub fn setup_subcommand_lock<T, F>(root: &Path, what: &str, f: F) -> Result<T> | |
| where | |
| F: FnOnce() -> Result<T>, | |
| { | |
| let _lock = acquire_mutation_lock(root).with_context(|| { | |
| format!( | |
| "Refusing destructive '{what}' on '{}': another Numan mutation is already in progress.", | |
| root.display() | |
| ) | |
| })?; | |
| eprintln!( | |
| "(audit) setup mutation lock acquired for {what} on '{}'.", | |
| root.display() | |
| ); | |
| f() | |
| } | |
| /// Acquire the root mutation lock, run a destructive setup subcommand, and | |
| /// release the lock on return. | |
| /// | |
| /// Every destructive setup entry point (install, off-path registration, | |
| /// PATH-Nu registration, managed removal, derive/active/upgrade via | |
| /// `numan use`) flows through this helper so the lock boundary has exactly | |
| /// one source of truth — closing PR `#69`'s WCr (`setup_family_mutation_lock`) | |
| /// and ensuring that a concurrent `numan use`, `numan install`, or | |
| /// `numan doctor --fix` cannot interleave filesystem mutations on the same | |
| /// root. | |
| /// | |
| /// `what` is a short human-readable label (e.g. `"Nushell install"`, | |
| /// `"off-path Nu registration"`, `"managed Nushell removal"`); it lands | |
| /// in the audit log alongside the `(audit)` prefix so safe-batch | |
| /// automation can grep one consistent shape across the whole destructive | |
| /// setup surface: | |
| /// | |
| /// |
🤖 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/util/fs_safety.rs` around lines 99 - 141, Update the error context in
setup_subcommand_lock so it identifies the refused destructive operation without
repeating the “another Numan mutation is already in progress” message supplied
by acquire_mutation_lock. Preserve the existing root and operation details, and
leave the audit logging and lock lifetime unchanged.
| // PR69 Srm: the injected installer must write the VERSIONED layout, | ||
| // exactly like the real network installer now does. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove PR references from code comments.
These comments refer to PR-specific review context. Keep only the non-obvious behavioral reason for the fixture or test.
As per coding guidelines, “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.”
Also applies to: 85-87, 352-356
🤖 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 `@tests/setup_nu_test.rs` around lines 62 - 63, Update the comments near the
injected installer and the referenced locations to remove PR-specific
identifiers and review context, retaining only concise explanations of the
non-obvious behavioral reason for the fixture or test.
Source: Coding guidelines
| // Stage a managed Nushell install under NUMAN_ROOT. | ||
| let managed = root.join("tools").join("nushell").join(bin_name); | ||
| stage_fake_nu(&nu_source, &managed); | ||
| assert!(managed.is_file(), "managed Nu must be on disk"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stage the current versioned managed Nu layout in both force-gate tests.
version_manager::version_binary defines the managed binary as <root>/tools/nushell/<version>/<bin>. These tests instead create the legacy <root>/tools/nushell/<bin> path. They can test legacy migration behavior instead of the current replacement gate.
Create the managed fixture with version_manager::version_binary(root, version). Write the active-version marker before execute_nu. Assert against that versioned binary after refusal and forced replacement.
As per coding guidelines, “Add or update tests for behavior changes, including relevant failure paths.”
Also applies to: 433-434
🤖 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 `@tests/setup_nu_test.rs` around lines 371 - 374, Update both force-gate tests
around stage_fake_nu and execute_nu to use version_manager::version_binary(root,
version) for the managed fixture, matching the current
<root>/tools/nushell/<version>/<bin> layout. Write the active-version marker
before invoking execute_nu, and assert against the versioned binary after both
refusal and forced replacement paths.
Source: Coding guidelines
2 participants
Updates the curated STARTERS list in
numan try:All 9 unit tests pass.