Repository navigation
fix(install): preview hooks during dry runs - #11010
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesDry-run hook previews
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 SummaryThis PR fixes
Confidence Score: 5/5Safe to merge — the change only reroutes dry-run hook calls through a new read-only preview path and is well-covered by new e2e tests. The bug fix is surgical: a single No files require special attention. Important Files Changed
Reviews (9): Last reviewed commit: "fix(install): preserve hook preview argu..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/cli/test_bootstrap (1)
321-324: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the hook is discovered to ensure the template rendered successfully.
If template rendering silently fails (e.g., due to an unregistered
execfunction throwing a Tera error), thefinalhook won't be defined in the internal configuration. The test will pass trivially because the hook command is never processed.Replace
assert_succeedwithassert_containsto verify that the dry-run output contains the preview of the hook's command. This strictly guarantees that the template successfully parsed and rendered without executing theexecfunction.💡 Proposed fix
-assert_succeed "mise bootstrap --dry-run" +assert_contains "mise bootstrap --dry-run" "touch template_exec_hook_ran" assert_fail "test -f template_exec_ran" assert_fail "test -f template_exec_hook_ran"🤖 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 `@e2e/cli/test_bootstrap` around lines 321 - 324, Update the dry-run assertion in the bootstrap test to use assert_contains and verify the output includes the rendered final hook command preview. Keep the existing file-absence assertions, and match the expected hook command text so the test confirms the hook was discovered and the template rendered successfully.
🤖 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.
Nitpick comments:
In `@e2e/cli/test_bootstrap`:
- Around line 321-324: Update the dry-run assertion in the bootstrap test to use
assert_contains and verify the output includes the rendered final hook command
preview. Keep the existing file-absence assertions, and match the expected hook
command text so the test confirms the hook was discovered and the template
rendered successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c892593-4f0b-4542-a42b-8eec97a4020b
📒 Files selected for processing (4)
e2e/cli/test_bootstrapsrc/cli/bootstrap.rssrc/system/files.rssrc/tera.rs
Summary
mise install --dry-runandmise bootstrap --dry-runfrom executing preinstall or postinstall hooksdry_runflag so hook enablement, shell matching, root matching, and platform selection cannot drift--dry-runand--dry-run-code, verifying hooks and tool installs have no side effectsRoot cause
The no-missing-tools path in
Install::install_missing_runtimescalled the postinstall hook directly, bypassing the dry-run guard used by tool installation.Validation
mise run lint-fixmise run test:e2e e2e/cli/test_bootstrap e2e/cli/test_install_dry_runcargo test --all-features hooks::tests::inline_command_display_preserves_argv_boundariescargo fmt --all -- --checkshellcheck -x e2e/cli/test_bootstrap e2e/cli/test_install_dry_runshfmt -d e2e/cli/test_bootstrap e2e/cli/test_install_dry_rungit diff --check