Repository navigation
Unify hook and alias shell execution - #2089
Conversation
Replace two separate execution functions (`execute_command_in_worktree` and `run_command_streaming`) with a single `execute_shell_command`. All three consumers (hooks, aliases, for-each) now share the same code path. Aliases and for-each gain signal forwarding (SIGINT/SIGTERM to child process group) and ANSI reset before command output. The `Cmd` builder gains a `.directive_file()` method that re-adds the env var after the security scrub, used by aliases to pass through shell integration directives. Deletes `CommandError` enum and `run_command_streaming` from for_each.rs. Call sites switch to `WorktrunkError::ChildProcessExited` downcasting. Phase 1 of the unify-hook-alias spec (Phase 2: alias pipeline support and Phase 3: foreground hook directive file are follow-ups). Co-Authored-By: Claude <noreply@anthropic.com>
Phase 2: Aliases now iterate `cmd_config.steps()` instead of flattening via `commands()`. Concurrent steps (named-table commands) spawn threads and run in parallel. Lazy `vars.*` expansion is supported in pipelines. Phase 3: Foreground hooks (all pre-* hooks; post-* with --foreground) pass the parent shell's directive file through to child processes, so inner `wt switch --create` in a hook body lands the shell in the new worktree. Background hooks continue to scrub the env var. Completes the unify-hook-alias spec. Removes implementation order section from spec (all phases done). Co-Authored-By: Claude <noreply@anthropic.com>
Update the spec diagram to use `execute_shell_command` (the actual implemented name), remove the proposed `CommandExecOptions` struct (individual parameters used instead), and fix the announcement section to reflect that pipeline summary is a follow-up, not yet implemented. Co-Authored-By: Claude <noreply@anthropic.com>
The architectural knowledge from the spec belongs in the code it describes. Expanded docstrings on `execute_shell_command` (consumer list, directive file trust model, ANSI reset rationale) and the `alias` module (pipeline execution model, lazy vars, trust model). Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
Nice consolidation — three call sites collapsing into execute_shell_command reads cleanly, and the directive-file pass-through model (scrub by default, opt-in for trusted contexts) is a good shape. A few items worth raising before merge:
codecov/patch is failing at 75.6% (20 missed lines). Per project policy this must be addressed or explicitly waived. The biggest gaps are in src/commands/alias.rs (the new AliasExecCtx::run method, the HookStep::Concurrent branch in run_alias_command, and the lazy template_references_var re-expansion path) and the spawn-failed branch in src/commands/for_each.rs. Concurrent alias execution and the spawn-failed JSON path in particular look genuinely uncovered, not just instrumentation noise.
Phase 3 lacks a regression test. The directive-file pass-through to foreground hooks is the security-relevant half of this PR — it enables wt switch --create inside a pre-start hook to actually land the user in the new worktree. The parallel alias change in #2077 added test_alias_passes_directive_file_to_subprocess; the equivalent for foreground hooks is missing. Without it, a future refactor could silently re-scrub WORKTRUNK_DIRECTIVE_FILE from foreground hooks and no test would catch it. A test that runs a foreground hook whose body writes WORKTRUNK_DIRECTIVE_FILE and asserts the parent file received the line would lock this in.
Cmd::run() silently ignores .directive_file(). Only Cmd::stream() re-adds the env var after the security scrub. A caller that builds a Cmd with .directive_file(path).run() will get a child with WORKTRUNK_DIRECTIVE_FILE unset and no warning. Today nothing in-tree calls .run() after .directive_file(), so this isn't a live bug, but it's a footgun for the next caller. Either mirror the re-add in run(), document the constraint on the builder, or debug_assert! in run() that directive_file.is_none().
Minor observations (non-blocking):
src/commands/for_each.rsspawn-failed path now prints the gutter-formatted error before the "Failed in …" header, where previously the header came first. Intentional?HookStep::Concurrentalias execution joins and propagates only the firstErr— a panic in any other thread is still surfaced viaexpect, but a returnedErrfrom a sibling step is dropped. Acceptable for this PR (matches single-step semantics) but worth a comment if we expect users to run failure-prone concurrent steps.for_eachJSONerrorfield now reads"exit status: 1"instead of"exit code 1". Snapshot tests are updated, but if anything downstream parses this it will break — flag in the changelog if there is one.
- Add `test_alias_concurrent_steps` — covers `HookStep::Concurrent` branch in alias execution (named-table form runs commands concurrently) - Add `test_foreground_hook_passes_directive_file` — regression test for Phase 3; verifies `wt switch --create` inside a foreground hook writes shell directives back to the parent - Add `debug_assert!` in `Cmd::run()` catching `.directive_file().run()` misuse (directive_file only applies in `.stream()`) - Fix for-each spawn-failed print order: header before detail Co-Authored-By: Claude <noreply@anthropic.com>
codecov/patch failure — false positive analysis
Method: queried
The integration tests Likely root cause: The two coverage gaps the prior review actually identified are now closed:
Patch coverage went from 75.6% → 81.32% as a result. The remainder is the rename artifact. Approval stands — flagging this so you can override the codecov gate consciously. |
Adds three additive tests on top of 8e9e1ac: * `test_for_each_json_spawn_failure` — exercises the spawn-failed branch of `step_for_each`'s failure handler in JSON mode (`exit_code: null`). PATH is pointed at a directory containing only a `git` symlink so wt can still run while the child shell spawn fails. * `test_alias_pipeline_lazy_vars` — covers the lazy `vars.` re-expansion path in `AliasExecCtx::run`, where a pipeline-form alias step references a var set by an earlier step. * `test_alias_concurrent_step_failure` — covers error propagation out of the `HookStep::Concurrent` join loop, complementing the happy-path `test_alias_concurrent_steps`.
|
Quick follow-up to the false-positive analysis above: pushed 1f26c165 with three additive tests that turned out to cover real (not just rename-artifact) gaps:
|
Extracts two helpers in command_executor.rs to consolidate duplication left over from PR #2089: - `expand_shell_template` replaces three lazy-expansion sites (hooks, aliases, pipeline runner) and the eager sibling in `expand_commands`, so every shell-command template expansion flows through one helper. - `wait_first_error` drains a sequence of Result items and returns the first error, letting alias's `thread::scope` block and the pipeline runner's concurrent group share "wait all, return first error" semantics over different concurrency primitives. Also folds `expand_now` and `build_step_context_json` in run_pipeline.rs into a single `step_context` helper returning `Cow`, eliminating a per-step double-clone of the context map. The concurrent-group bail format now matches the serial-step format ("command failed with {exit}: {label}"), removing a minor inconsistency. Co-Authored-By: Claude <noreply@anthropic.com>
…2095) Two small follow-ups to #2089, which unified shell command execution for hooks and aliases but left some duplication on the template-expansion and concurrent-wait paths. ## What changed **`expand_shell_template`** (in `src/commands/command_executor.rs`) — one helper that builds the `&str` vars map and calls `expand_template` with `shell_escape=true`. Replaces three lazy-expansion sites (foreground hooks, background pipeline runner, alias runner) and the eager sibling inside `expand_commands`, so every shell template expansion now flows through one helper. **`wait_first_error`** (same file) — drains a sequence of `Result<(), E>` items and returns the first error. Alias's `thread::scope` block and the pipeline runner's concurrent group now share "wait all, return first error" semantics over different concurrency primitives (in-process threads vs OS subprocesses). The concurrent-group bail format now matches the serial-step format (`command failed with {exit}: {label}`), removing a minor inconsistency — per-command output already lives in each command's log file. **`step_context` in `run_pipeline.rs`** — folds `expand_now` and `build_step_context_json` into one helper returning `Cow<HashMap<String, String>>`. Unnamed steps borrow the base context; named steps clone once and insert `hook_name`. Previously, named steps cloned twice (once for expansion, once for JSON). ## Where to look - `src/commands/command_executor.rs` — two new helpers - `src/commands/hooks.rs` — deleted local `expand_lazy_template` - `src/commands/alias.rs` — concurrent block and `AliasExecCtx::run` - `src/commands/run_pipeline.rs` — `step_context`, `run_concurrent_group`, `expand_commands` in main loop ## Testing `cargo run -- hook pre-merge --yes` is green: 3025/3025 insta-nextest tests, pre-commit lints, doctests, `cargo doc`. No snapshots changed. Co-authored-by: Claude <noreply@anthropic.com>
## Summary Two follow-ups to the recently-merged unify-hook-alias work (#2089): **Pipeline announcement.** Aliases now print a structured summary of their pipeline (e.g., `Running alias deploy: install; build, lint`) instead of the bare `Running alias deploy`. Hook and alias formatters share a new `format_pipeline_summary_from_names` helper that takes per-step names plus label closures — hooks pass `source:` prefixes with unnamed-count fallbacks, aliases pass plain names and skip unnamed (no natural fallback label like `user`/`project`). **Concurrent execution stays separate.** I considered unifying `AliasExecCtx::run` (uses `thread::scope` because `execute_shell_command` is a blocking streaming call) with `run_concurrent_group` in `run_pipeline.rs` (uses raw `Child` handles + log-file redirection). The leaf primitives diverge — one needs threads because the streaming executor blocks, the other uses processes directly — and bridging them requires either making the streaming executor non-blocking (entangled with signal forwarding, ANSI reset, `Cmd` tracing) or threading background commands that don't need it. A module-level doc comment in `alias.rs` explains the divergence so the next person doesn't repeat the analysis. **Test escape hatch.** Adds `WORKTRUNK_TEST_SERIAL_CONCURRENT=1` honored by both concurrent paths, mirroring the `RAYON_NUM_THREADS=1` pattern in `step_prune` tests. Lets snapshot tests use meaningful command output (e.g., `echo INSTALL/BUILD/LINT`) and still produce a deterministic ordering. ## Test plan - [x] 5 unit tests for `format_alias_announcement` (single unnamed, all-unnamed pipeline, concurrent named, pipeline named, mixed named/unnamed) - [x] Integration snapshot test `test_alias_pipeline_announcement` exercising the alias path - [x] Two integration tests in `post_start_commands.rs` (`test_post_start_concurrent_serial_force`, `test_post_start_concurrent_serial_bails_on_failure`) exercising the background pipeline runner's serial branch — happy path (deterministic append ordering) and failure path (bail-on-first, second never runs) - [x] Existing hook formatter tests still cover the source-prefixed path via the shared helper - [x] Full `wt hook pre-merge --yes` (3030 tests, all green) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
) ## Problem `wt <alias>` writes the alias body's stdout onto wt's stderr instead of stdout, so `wt my-alias | tr …` (or any other downstream pipe) silently produces no output. Reported in #2478: ```sh # config.toml [aliases] test-output = "echo 'hello'" $ wt test-output | tr '[:lower:]' '[:upper:]' hello # ← should be HELLO ``` Root cause is in `src/output/handlers.rs::execute_shell_command`, which `Cmd::shell(...).stdout(Stdio::from(io::stderr()))` for *every* foreground command. The redirect was originally a hook-only behavior added for deterministic ordering with wt's own stderr "Running …" messages; PR #2089 unified hooks, aliases, and `for-each` onto this shared executor and inherited the redirect uniformly without distinguishing aliases. ## Solution Thread a `redirect_stdout_to_stderr` bool through `ForegroundStep` and `execute_shell_command`. Hooks (`src/commands/hooks.rs`) and `wt step for-each` (`src/commands/for_each.rs`) keep the merge (`true`) — their output is decoration around wt's stderr messages, and merging keeps ordering deterministic. Aliases (`src/commands/alias.rs`) pass through (`false`) — `wt <alias>` is a user-defined command and its stdout must remain pipeable. Concurrent alias steps still write through `output/concurrent.rs`'s prefixed-line stderr consumer — the gutter rendering (`build │ BUILD`) inherently belongs on stderr, so that path is unchanged. ## Testing - Added `test_alias_body_writes_to_stdout` in `tests/integration_tests/step_alias.rs`. It runs an alias whose body is `echo hello` and asserts the output appears on stdout, not stderr. Failed before the fix; passes after. - Existing snapshot tests for `step_alias` pick up the routing change — diffs are limited to lines moving from `----- stderr -----` to `----- stdout -----`, which is the user-visible behavior we want. Updated via `cargo insta test --accept`. - Manual end-to-end check: `wt -y test-output | tr '[:lower:]' '[:upper:]'` now prints `HELLO`. - Full `cargo test --test integration` (1569 tests) and `cargo test --lib --bins` (606 tests) pass. --- Closes #2478 — automated triage Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Hooks, aliases, and
for-eachall run shell commands in a worktree but used two separate execution functions with different capabilities. This collapses them into one (execute_shell_command), then builds on the shared foundation.Phase 1 —
run_command_streamingandCommandErrordeleted. All three consumers callexecute_shell_command(renamed fromexecute_command_in_worktree) with a newdirective_fileparameter.Cmdgains a.directive_file()builder method that re-adds the env var after the security scrub. Aliases andfor-eachgain signal forwarding and ANSI reset.Phase 2 — Aliases iterate
cmd_config.steps()instead of flattening viacommands().HookStep::Concurrentsteps spawn threads viathread::scope. Lazyvars.*expansion supported in pipelines.Phase 3 — Foreground hooks (pre-*, post-* with
--foreground) pass the directive file through to child processes.wt switch --createinside a pre-start hook body now lands the shell in the new worktree. Background hooks continue to scrub.Follow-up: alias announcements could show pipeline summary (e.g.,
Running alias deploy: install; build, lint).