Repository navigation
fix: deduplicate repeated shell arguments in min completions install - #1283
norrietaylor merged 2 commits into
Conversation
min completions install iterated the shell argument list as given, so a repeated name (`min completions install zsh zsh`) wrote the file and printed its installed path once per copy. Resolve the argument list to its unique shells, keeping first-seen order, before the install loop.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthrough
ChangesInstallation target normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change makes repeated shell arguments install and print each shell only once while preserving existing behavior for other requests. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the behavior change, affected command, expected results, and verification steps. It does not use the template's ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…-ac27262456619fb8
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; policy.actions.approve is off.
Scope: matches the stated intent exactly — one small fix (install_targets) plus its test, nothing else touched. Clobber: none; git log on this file shows a clean history with no revert of recent work. Correctness: verified the only caller (lib.rs:829) passes an undeduplicated Vec<InstallShell> straight from clap, so the bug (repeated shell installed/printed once per occurrence) was real; the fix preserves first-seen order and the empty-means-ALL case, and the new test covers dedup, order, and the empty-request path. InstallShell already derives Eq/PartialEq so Vec::contains works as used, and with only 3 possible shells the O(n²) scan is a non-issue. Nothing to flag.
What ran
4 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. rg for InstallShell/cmd_install call sites outside completions.rs — exit 0 · self-reported
Only caller is crates/minimal/src/lib.rs:829 (
CompletionsCommand::Installarm), passing&args.shellsstraight from a plainVec<InstallShell>positional clap arg with no prior dedup — confirms the bug was real and this is the only call site affected.
2. Read InstallShell enum definition and ALL const (completions.rs:29-38) — exit 0 · self-reported
InstallShell already derives PartialEq, Eq (needed by Vec::contains in the new install_targets), and ALL is a fixed 3-element array (Bash, Zsh, Fish), so the O(n^2) contains-based dedup is trivially bounded — not a perf concern.
3. git log -15 -- crates/minimal/src/completions.rs — exit 0 · self-reported
History is feat(#1009) split print/install, fix(#1082) unloadable-shell flag, then this fix. No revert of unrelated recent work; scope matches the file's own history.
4. grep for itertools/IndexSet usage elsewhere in the minimal crate as a possible existing dedup helper — exit 0 · self-reported
No itertools dependency and no existing order-preserving-dedup helper in this crate (only unordered HashSet usage in loadouts.rs) — the hand-rolled loop isn't reinventing an available utility.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-5 via claude-code · prompt review-prompt/v2 · head 8418d57266ad · verdict COMMENT · 0 finding(s), 0 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABKmgzFw
min completions installiterated its shell argument list exactly asreceived, so naming the same shell more than once (e.g.
min completions install zsh zsh) installed that shell once per occurrence: the completionfile was written twice and its path printed twice on stdout. The install
loop now runs over the unique shells, preserving first-seen order, so each
named shell is installed — and its path printed — exactly once. An empty
request (a bare
install, meaning every installable shell) and requestsnaming distinct shells are unaffected.
Verification
cargo fmt --all --check --manifest-path target/Cargo.toml — clean
cargo clippy --workspace --locked --manifest-path target/Cargo.toml -- -D warnings — clean, no warnings
cargo build --workspace --locked --manifest-path target/Cargo.toml — ok
cargo test --workspace --locked --manifest-path target/Cargo.toml — all pass; new test completions::tests::install_targets_dedupes_preserving_order
Summary by CodeRabbit