fix(watch): forward --wrap-process to watchexec#10392
Conversation
mise watch accepts --wrap-process (it mirrors watchexec's options) but never forwarded it when building the watchexec command line, so e.g. `mise watch --wrap-process none foo` actually ran `watchexec --restart -- mise run foo` and watchexec fell back to its default `group` wrap mode. A task launching a TUI then ended up in a non-foreground process group and blocked on terminal I/O, hanging -- while the equivalent direct `watchexec --wrap-process none ...` worked (jdx#10212). Forward --wrap-process when it differs from watchexec's default (group), and add PartialEq + a kebab-case strum::Display to WrapMode (mirroring OnBusyUpdate) so it serializes to watchexec's accepted values (group/session/none). Add a unit test pinning those serialized values. Addresses discussion jdx#10212. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates ChangesWatch wrap-process flag forwarding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Greptile SummaryThis PR fixes a bug where
Confidence Score: 5/5Safe to merge — single-file change that closes a clear gap between the parsed CLI flag and the subprocess invocation. The fix is minimal and well-scoped: it adds two derives to an enum, inserts one conditional push into an existing args-building block, and adds a unit test that pins the exact strings forwarded to watchexec. The pattern is identical to the already-working OnBusyUpdate forwarding, the default-comparison logic is correct, and strum's kebab-case serialization produces the values watchexec expects. No regressions are plausible. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix(watch): forward --wrap-process to wa..." | Re-trigger Greptile |
Problem
mise watch --restart --wrap-process none <task>hangs when the task launches a TUI, while the equivalent directwatchexec --restart --wrap-process none mise run <task>works fine. Reported in #10212.Root cause
mise watchaccepts--wrap-process(it mirrors watchexec''s options for help/completion), butWatch::run()never forwards the flag when building the watchexec command line — a repo-wide search findswrap_processonly in the clap definition. Somise watch --wrap-process none fooactually runs:and watchexec falls back to its default
--wrap-process group. Ingroupmode the TUI ends up in a non-foreground process group and blocks on terminal I/O, hence the hang.Fix
--wrap-processinWatch::run()when it differs from watchexec''s default (group), right next to the existing--on-busy-updateforwarding.PartialEqand a kebab-casestrum::DisplaytoWrapMode(mirroringOnBusyUpdate) so it serializes to watchexec''s accepted values:group/session/none.Testing
WrapMode''s serialized values (group/session/none) — these are forwarded verbatim to watchexec.mise watch --restart --wrap-process none <task>underMISE_DEBUGnow shows--wrap-process nonein the spawnedwatchexeccommand, and the TUI no longer hangs. (The argv construction lives in the long-runningWatch::run(), which isn''t covered by the existing e2e suite.)cargo fmt --all -- --checkpasses.Note: several other watchexec flags are similarly parsed-but-not-forwarded (e.g.
-i/--ignore,--workdir); this PR is scoped to--wrap-process(the reported bug) and those can be a follow-up.Addresses #10212
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
mise watchnow correctly forwards the--wrap-processoption to the process watcher instead of silently dropping it, ensuring process wrapping behavior is applied as specified.Tests