Repository navigation
refactor(cli): keep one copy of the task-flag escape rule - #12343
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe CLI now uses a shared helper to escape task-side flags. Lone hyphens and ordinary arguments remain unchanged. Separator-tail handling uses the shared path. Unit tests cover escaping and unescaping behavior. ChangesTask flag escaping
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change centralizes an existing CLI argument-escaping rule without intended behavior changes; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryThe PR centralizes task-side flag escaping without changing its intended behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "refactor(cli): keep one copy of the task..." | Re-trigger Greptile |
c6411ed to
107b3bf
Compare
### 🚀 Features - **(bootstrap)** add package plugin uninstall support by @jdx in [#12332](#12332) ### 🐛 Bug Fixes - **(brew-cask)** recognize Homebrew-owned casks by @donbeave in [#12346](#12346) - **(cli)** report a cd target it cannot enter instead of panicking by @JamBalaya56562 in [#12314](#12314) - **(config)** skip disabled idiomatic tracked configs by @xqm32 in [#12194](#12194) - **(config)** keep comments when saving from mise edit by @Marukome0743 in [#12319](#12319) - **(config)** strip a byte-order mark before reading a version file by @JamBalaya56562 in [#12325](#12325) - **(doctor)** report installs that provide no executables by @Marukome0743 in [#12321](#12321) - **(env)** say why an age SSH identity could not be used by @Marukome0743 in [#12339](#12339) - **(env)** name the key and file behind an unexpanded $VAR by @Marukome0743 in [#12316](#12316) - **(go)** don't hand go install a GOROOT from another Go by @Marukome0743 in [#12342](#12342) - **(task)** treat a SIGINT-killed child as an interruption by @Marukome0743 in [#12323](#12323) - **(task)** preserve string type for usage value flags by @jdx in [#12355](#12355) - **(tasks)** tell Windows users why a task file was skipped by @JamBalaya56562 in [#12324](#12324) - **(tool-stub)** keep env_with_path instead of rebuilding PATH by @tmkx in [#12322](#12322) - **(watch)** restore the controlling terminal from a drop guard by @Marukome0743 in [#12328](#12328) ### 🚜 Refactor - **(cli)** keep one copy of the task-flag escape rule by @JamBalaya56562 in [#12343](#12343) ### 📚 Documentation - **(tasks)** point extensionless pwsh tasks at MISE_TASK_DIR by @JamBalaya56562 in [#12313](#12313) ### 📦️ Dependency Updates - update rust crate aube-registry to v2.1.0 by @renovate[bot] in [#12348](#12348) - bump aube to 2.1.0 by @jdx in [#12351](#12351) - bump usage to 6.2.0 by @jdx in [#12354](#12354) ### 📦 Registry - fix usage version test by @jdx in [#12349](#12349) - install oc from a channel directory by @Marukome0743 in [#12326](#12326) ### Chore - **(ci)** use github runners for fork prs by @jdx in [#12345](#12345) - **(ci)** prevent cache warmer starvation by @jdx in [#12350](#12350) ### New Contributors - @tmkx made their first contribution in [#12322](#12322)
src/cli/mod.rscarried the same rule three times: prefix an argument withTASK_ARG_ESCAPE_PREFIXwhen it
starts_with('-') && arg != "-", otherwise pass it through.escape_args_after_separator(466-476)escape_task_args, separator branch (621-629)This came from review, not from me. CodeRabbit raised it on #12327 and again on #12314. Both
times it fell outside the diff — those PRs change one hunk each in this file, ~200 lines away — so I
declined and said it wanted its own PR. A finding that keeps landing on unrelated PRs is asking
for a home, and the second report is what turned up the third copy, which the first had not
mentioned.
No behaviour change is intended.
The shape
An iterator-returning helper covers the two slice sites but not the third, which applies the rule to
a single argument inside a larger
while. A singular helper covers all three:The separator branch then does not need it at all — it can delegate:
One correction to the review note, which said
escape_args_after_separator"cannot be reuseddirectly here". It can.
separator_idxis found by matching"--", so for the slice beginningthere,
escape_args_after_separator(.., 0)emitsslice[..=0]— exactly["--"]— and then runsthe same escape over the tail. That is what the branch built by hand, element for element.
The predicate now appears once.
grep -cfor it in the file returns1.Tests
Existing unit tests already reach all three sites, and they are in a
#[cfg(test)] mod testswithno
#[cfg(unix)], so they run on Windows CI too:test_escape_task_args_preserves_task_separator_tail—run atask -q -- -- --help. Asserts the-qbefore the separator (the task-args loop) and both elements after it (the separatorbranch).
test_escape_task_args_preserves_naked_task_separator_tail— the form with norun, which entersescape_args_after_separatordirectly...._splits_attached_short_option_values_before_task,..._preserves_equals_attached_option_valuesand
..._keeps_hyphen_prefixed_attached_value_bound_to_optionexercise the task-args loop.One test is added, for the rule itself. The
--is-not-a-flag exception was carried by all threecopies and asserted by none; now that it has one home, it is pinned there — including that a lone
-survives, which is what a task reading stdin depends on.No e2e: nothing observable changes.
Verification
cargo fmt --allis clean. Nothing here has been compiled or run locally —cargo checkdoesnot work on this machine (
libz-ng-syswantscmake, which is not installed), so for a refactorthis is thinner than I would like and CI is the first thing to type-check it. What stands in for it
is the existing coverage above, and the equivalence argument for the delegating branch, which is a
reading of
args[separator_idx] == "--"rather than a run.Summary by CodeRabbit
Bug Fixes
Tests