diff --git a/CHANGELOG.md b/CHANGELOG.md index eac162b8e..a923e4fb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,8 @@ - **`wt config approvals add --yes` records approvals without a terminal**: the command refused every non-interactive run, so a container or CI job could not pre-approve a project it had just cloned. `--yes` now lists what it trusts and writes it, and a save it cannot make fails the command rather than warning and exiting 0. ([#3819](https://github.com/max-sixty/worktrunk/pull/3819)) +- **A foreground hook keeps the terminal, so it can ask before continuing**: A `pre-*` hook now inherits wt's stdin exactly as an alias body already did, so `trust = "gum confirm 'trust this worktree?' && mise trust"` gets a prompt instead of an immediate EOF. That covers every type under `wt hook --foreground` too. (Breaking: hooks no longer receive the JSON context on stdin at all — a hook that ran `json.load(sys.stdin)` must take the values it needs as template variables instead, e.g. `setup = "python3 setup.py {{ branch }} {{ repo }}"`, which every hook has always had. The forms that can't hold a terminal — detached `post-*` hooks, and the children of a concurrent group, who would race for one — now read EOF rather than JSON. Foreground steps share one stdin, so a step that drains it to EOF starves the steps behind it when that stdin is a pipe or a file.) Fixes [#3093](https://github.com/max-sixty/worktrunk/issues/3093). ([#3129](https://github.com/max-sixty/worktrunk/pull/3129)) + - **`remote_repo` names the repository as the remote spells it**: `repo` is the directory on disk, so a renamed clone reports the new name. `{{ remote_repo }}` takes it from the primary remote's URL, available everywhere `owner` is and unset when no remote parses. ([#3745](https://github.com/max-sixty/worktrunk/pull/3745), thanks @canac) ### Fixed diff --git a/docs/src/content/docs/extending.md b/docs/src/content/docs/extending.md index 4d16da16f..d3afb8dea 100644 --- a/docs/src/content/docs/extending.md +++ b/docs/src/content/docs/extending.md @@ -240,7 +240,7 @@ Aside from the differences below, hooks and aliases behave the same. | Force-bind escape | `--var KEY=VALUE` (deprecated in favor of `--KEY=VALUE`, but still force-binds) | None; smart routing is the only path | | `--help` | `wt hook --help` lists hook types; `wt hook --help` shows flags and arguments for that type | The template body is the documentation: `wt --help` redirects to `wt config alias show` / `dry-run`. `wt --help` and `wt step --help` list configured aliases alongside built-in commands | | Inspection | `wt hook show [type] [--expanded]` | `wt config alias show ` / `wt config alias dry-run ` | -| Stdin | All template variables as JSON (parse with `json.load(sys.stdin)`) | Inherits parent stdin (pipes pass through; interactive TUIs like `wt switch` keep the tty) | +| Stdin | A hook running in the foreground inherits parent stdin, same as aliases (interactive prompts work) — `pre-*` hooks, and any type under `wt hook --foreground`; detached `post-*` hooks and concurrent children read EOF | Inherits parent stdin (pipes pass through; interactive TUIs like `wt switch` keep the tty) | | Template-context extras | `hook_type`, `hook_name`, per-type operation vars (`base`, `target`, `pr_number`, …) | `args` on top of the shared base variables | diff --git a/docs/src/content/docs/hook.md b/docs/src/content/docs/hook.md index 1b298fe5f..99156c961 100644 --- a/docs/src/content/docs/hook.md +++ b/docs/src/content/docs/hook.md @@ -247,19 +247,30 @@ The `worktree_path_of_branch` function returns the filesystem path of a worktree setup = "cp {{ worktree_path_of_branch('main') }}/config.local {{ worktree_path }}" ``` -## JSON context +## Interactive hooks -Hooks receive all template variables as JSON on stdin, enabling complex logic that templates can't express. Variables that are unset in a template are absent from the JSON too, so read the optional ones with a default — `branch` has none in a detached worktree: +A hook running in the foreground inherits wt's stdin, so it can ask before continuing: ```toml [pre-start] -setup = "python3 scripts/pre-start-setup.py" +trust = "gum confirm 'trust this worktree?' && mise trust" +``` + +That covers `pre-*` hooks and any type under `wt hook --foreground`, except where the hook is a concurrent group — a table with two or more keys, whose children would race for the terminal, so each reads EOF instead. A detached `post-*` hook reads EOF too, having no terminal at all. Nothing is ever piped in — a hook reads its context through template variables, whatever form it runs in. + +Foreground steps run in order and share one stdin, so a step that drains it to EOF — a `cat`, a `read` — leaves nothing for the steps behind it when that stdin is a pipe or a file. Under a terminal each step can prompt in turn. Steps accumulate across config files, so a user `[pre-start]` and a project `[pre-start]` form one pipeline. + +Logic that templates can't express belongs in a script, with the values it needs passed as arguments: + +```toml +[post-start] +setup = "python3 scripts/post-start-setup.py {{ branch }} {{ repo }}" ``` ```python -import json, sys, subprocess -ctx = json.load(sys.stdin) -if ctx.get('branch', '').startswith('feature/') and 'backend' in ctx['repo']: +import subprocess, sys +branch, repo = sys.argv[1], sys.argv[2] +if branch.startswith('feature/') and 'backend' in repo: subprocess.run(['make', 'seed-db']) ``` diff --git a/plugins/worktrunk/skills/worktrunk/reference/extending.md b/plugins/worktrunk/skills/worktrunk/reference/extending.md index e07d479a5..916a785e1 100644 --- a/plugins/worktrunk/skills/worktrunk/reference/extending.md +++ b/plugins/worktrunk/skills/worktrunk/reference/extending.md @@ -236,7 +236,7 @@ Aside from the differences below, hooks and aliases behave the same. | Force-bind escape | `--var KEY=VALUE` (deprecated in favor of `--KEY=VALUE`, but still force-binds) | None; smart routing is the only path | | `--help` | `wt hook --help` lists hook types; `wt hook --help` shows flags and arguments for that type | The template body is the documentation: `wt --help` redirects to `wt config alias show` / `dry-run`. `wt --help` and `wt step --help` list configured aliases alongside built-in commands | | Inspection | `wt hook show [type] [--expanded]` | `wt config alias show ` / `wt config alias dry-run ` | -| Stdin | All template variables as JSON (parse with `json.load(sys.stdin)`) | Inherits parent stdin (pipes pass through; interactive TUIs like `wt switch` keep the tty) | +| Stdin | A hook running in the foreground inherits parent stdin, same as aliases (interactive prompts work) — `pre-*` hooks, and any type under `wt hook --foreground`; detached `post-*` hooks and concurrent children read EOF | Inherits parent stdin (pipes pass through; interactive TUIs like `wt switch` keep the tty) | | Template-context extras | `hook_type`, `hook_name`, per-type operation vars (`base`, `target`, `pr_number`, …) | `args` on top of the shared base variables | diff --git a/plugins/worktrunk/skills/worktrunk/reference/hook.md b/plugins/worktrunk/skills/worktrunk/reference/hook.md index 2cde87d8e..4975d24c7 100644 --- a/plugins/worktrunk/skills/worktrunk/reference/hook.md +++ b/plugins/worktrunk/skills/worktrunk/reference/hook.md @@ -241,19 +241,30 @@ The `worktree_path_of_branch` function returns the filesystem path of a worktree setup = "cp {{ worktree_path_of_branch('main') }}/config.local {{ worktree_path }}" ``` -## JSON context +## Interactive hooks -Hooks receive all template variables as JSON on stdin, enabling complex logic that templates can't express. Variables that are unset in a template are absent from the JSON too, so read the optional ones with a default — `branch` has none in a detached worktree: +A hook running in the foreground inherits wt's stdin, so it can ask before continuing: ```toml [pre-start] -setup = "python3 scripts/pre-start-setup.py" +trust = "gum confirm 'trust this worktree?' && mise trust" +``` + +That covers `pre-*` hooks and any type under `wt hook --foreground`, except where the hook is a concurrent group — a table with two or more keys, whose children would race for the terminal, so each reads EOF instead. A detached `post-*` hook reads EOF too, having no terminal at all. Nothing is ever piped in — a hook reads its context through template variables, whatever form it runs in. + +Foreground steps run in order and share one stdin, so a step that drains it to EOF — a `cat`, a `read` — leaves nothing for the steps behind it when that stdin is a pipe or a file. Under a terminal each step can prompt in turn. Steps accumulate across config files, so a user `[pre-start]` and a project `[pre-start]` form one pipeline. + +Logic that templates can't express belongs in a script, with the values it needs passed as arguments: + +```toml +[post-start] +setup = "python3 scripts/post-start-setup.py {{ branch }} {{ repo }}" ``` ```python -import json, sys, subprocess -ctx = json.load(sys.stdin) -if ctx.get('branch', '').startswith('feature/') and 'backend' in ctx['repo']: +import subprocess, sys +branch, repo = sys.argv[1], sys.argv[2] +if branch.startswith('feature/') and 'backend' in repo: subprocess.run(['make', 'seed-db']) ``` diff --git a/skills/worktrunk/reference/extending.md b/skills/worktrunk/reference/extending.md index e07d479a5..916a785e1 100644 --- a/skills/worktrunk/reference/extending.md +++ b/skills/worktrunk/reference/extending.md @@ -236,7 +236,7 @@ Aside from the differences below, hooks and aliases behave the same. | Force-bind escape | `--var KEY=VALUE` (deprecated in favor of `--KEY=VALUE`, but still force-binds) | None; smart routing is the only path | | `--help` | `wt hook --help` lists hook types; `wt hook --help` shows flags and arguments for that type | The template body is the documentation: `wt --help` redirects to `wt config alias show` / `dry-run`. `wt --help` and `wt step --help` list configured aliases alongside built-in commands | | Inspection | `wt hook show [type] [--expanded]` | `wt config alias show ` / `wt config alias dry-run ` | -| Stdin | All template variables as JSON (parse with `json.load(sys.stdin)`) | Inherits parent stdin (pipes pass through; interactive TUIs like `wt switch` keep the tty) | +| Stdin | A hook running in the foreground inherits parent stdin, same as aliases (interactive prompts work) — `pre-*` hooks, and any type under `wt hook --foreground`; detached `post-*` hooks and concurrent children read EOF | Inherits parent stdin (pipes pass through; interactive TUIs like `wt switch` keep the tty) | | Template-context extras | `hook_type`, `hook_name`, per-type operation vars (`base`, `target`, `pr_number`, …) | `args` on top of the shared base variables | diff --git a/skills/worktrunk/reference/hook.md b/skills/worktrunk/reference/hook.md index 2cde87d8e..4975d24c7 100644 --- a/skills/worktrunk/reference/hook.md +++ b/skills/worktrunk/reference/hook.md @@ -241,19 +241,30 @@ The `worktree_path_of_branch` function returns the filesystem path of a worktree setup = "cp {{ worktree_path_of_branch('main') }}/config.local {{ worktree_path }}" ``` -## JSON context +## Interactive hooks -Hooks receive all template variables as JSON on stdin, enabling complex logic that templates can't express. Variables that are unset in a template are absent from the JSON too, so read the optional ones with a default — `branch` has none in a detached worktree: +A hook running in the foreground inherits wt's stdin, so it can ask before continuing: ```toml [pre-start] -setup = "python3 scripts/pre-start-setup.py" +trust = "gum confirm 'trust this worktree?' && mise trust" +``` + +That covers `pre-*` hooks and any type under `wt hook --foreground`, except where the hook is a concurrent group — a table with two or more keys, whose children would race for the terminal, so each reads EOF instead. A detached `post-*` hook reads EOF too, having no terminal at all. Nothing is ever piped in — a hook reads its context through template variables, whatever form it runs in. + +Foreground steps run in order and share one stdin, so a step that drains it to EOF — a `cat`, a `read` — leaves nothing for the steps behind it when that stdin is a pipe or a file. Under a terminal each step can prompt in turn. Steps accumulate across config files, so a user `[pre-start]` and a project `[pre-start]` form one pipeline. + +Logic that templates can't express belongs in a script, with the values it needs passed as arguments: + +```toml +[post-start] +setup = "python3 scripts/post-start-setup.py {{ branch }} {{ repo }}" ``` ```python -import json, sys, subprocess -ctx = json.load(sys.stdin) -if ctx.get('branch', '').startswith('feature/') and 'backend' in ctx['repo']: +import subprocess, sys +branch, repo = sys.argv[1], sys.argv[2] +if branch.startswith('feature/') and 'backend' in repo: subprocess.run(['make', 'seed-db']) ``` diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 69f200b9b..0fdc964bd 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -1803,19 +1803,30 @@ The `worktree_path_of_branch` function returns the filesystem path of a worktree setup = "cp {{ worktree_path_of_branch('main') }}/config.local {{ worktree_path }}" ``` -## JSON context +## Interactive hooks -Hooks receive all template variables as JSON on stdin, enabling complex logic that templates can't express. Variables that are unset in a template are absent from the JSON too, so read the optional ones with a default — `branch` has none in a detached worktree: +A hook running in the foreground inherits wt's stdin, so it can ask before continuing: ```toml [pre-start] -setup = "python3 scripts/pre-start-setup.py" +trust = "gum confirm 'trust this worktree?' && mise trust" +``` + +That covers `pre-*` hooks and any type under `wt hook --foreground`, except where the hook is a concurrent group — a table with two or more keys, whose children would race for the terminal, so each reads EOF instead. A detached `post-*` hook reads EOF too, having no terminal at all. Nothing is ever piped in — a hook reads its context through template variables, whatever form it runs in. + +Foreground steps run in order and share one stdin, so a step that drains it to EOF — a `cat`, a `read` — leaves nothing for the steps behind it when that stdin is a pipe or a file. Under a terminal each step can prompt in turn. Steps accumulate across config files, so a user `[pre-start]` and a project `[pre-start]` form one pipeline. + +Logic that templates can't express belongs in a script, with the values it needs passed as arguments: + +```toml +[post-start] +setup = "python3 scripts/post-start-setup.py {{ branch }} {{ repo }}" ``` ```python -import json, sys, subprocess -ctx = json.load(sys.stdin) -if ctx.get('branch', '').startswith('feature/') and 'backend' in ctx['repo']: +import subprocess, sys +branch, repo = sys.argv[1], sys.argv[2] +if branch.startswith('feature/') and 'backend' in repo: subprocess.run(['make', 'seed-db']) ``` diff --git a/src/commands/alias.rs b/src/commands/alias.rs index ca3098b2b..59205c607 100644 --- a/src/commands/alias.rs +++ b/src/commands/alias.rs @@ -330,8 +330,9 @@ fn format_alias_announcement(name: &str, entry: &AliasEntry) -> Option { /// `load_aliases` returns (a name only appears in the map if some source /// defines it). When both are set, both bodies run — user first, then /// project — and each runs under its own trust regime: user steps skip -/// approval and pass EXEC through (issue #2101); project steps require -/// approval and scrub EXEC. +/// approval, project steps require it. Directive passthrough no longer +/// varies by source: the EXEC directive file is retired (#3977), so every +/// child scrubs it and only the CD file is passed through. pub(crate) struct AliasEntry { pub user: Option, pub project: Option, diff --git a/src/commands/command_executor.rs b/src/commands/command_executor.rs index b77df4b86..183be0e9b 100644 --- a/src/commands/command_executor.rs +++ b/src/commands/command_executor.rs @@ -32,7 +32,7 @@ pub struct PreparedCommand { /// read fresh from git config. pub template: String, /// Template variables, frozen at preparation. Serialized to JSON only at - /// the process boundary (child stdin, background pipeline spec). + /// the process boundary (the background pipeline spec). pub context: TemplateContext, /// Name used in template expansion errors: `"user:foo"` for named hook /// commands, `"user pre-merge hook"` for unnamed ones, the alias name for @@ -43,13 +43,6 @@ pub struct PreparedCommand { pub label: String, } -impl PreparedCommand { - /// The JSON form of `context` piped to the child's stdin. - pub fn context_json(&self) -> String { - self.context.to_json() - } -} - /// A step in a prepared pipeline, mirroring `HookStep`. #[derive(Debug, Serialize, Deserialize)] pub enum PreparedStep { @@ -92,8 +85,9 @@ pub type ErrorWrapper = Box) -> any /// /// Supplied at conversion time (`sourced_steps_to_foreground`) so a single /// `SourcedStep` shape can be produced by both alias and hook resolution. -/// Drives the per-step trust model (EXEC passthrough), announce policy, -/// stdin handling, and error wrapping. Hook-only metadata +/// Drives announce policy, stdout redirection, and error wrapping at +/// conversion time, plus the `commands.jsonl` trace label (`log_label`) and +/// git-discovery scrubbing (`is_hook`) at execution. Hook-only metadata /// (`hook_type`, `display_path`) lives on the `Hook` variant — it's /// per-pipeline, not per-step, so the per-step shape stays neutral. #[derive(Clone)] @@ -134,10 +128,6 @@ pub struct ForegroundStep { /// line plus the bash gutter; aliases stay silent (the caller emits one /// pipeline summary line). pub announce: PipelineKind, - /// Pipe `context_json` to the child's stdin (hooks); when `false`, inherit - /// the parent's stdin so interactive children keep the controlling tty - /// (aliases). - pub pipe_stdin: bool, /// Merge the child's stdout onto wt's stderr (`true`, hooks) or pass it /// through unchanged (`false`, aliases). Hooks merge so their output stays /// ordered with wt's own stderr "Running …" lines; aliases pass through so @@ -145,11 +135,11 @@ pub struct ForegroundStep { pub redirect_stdout_to_stderr: bool, /// Wraps a per-command failure into the final error returned to the caller. pub error_wrapper: ErrorWrapper, - /// Per-step directive passthrough. Trust differs by source — user-source - /// alias steps pass EXEC through (the body is the user's own config), - /// while project-source steps and all hook steps scrub it. Per-step rather - /// than per-pipeline so a merged user+project alias relaxes the user's - /// own steps without leaking the project's body into the parent shell. + /// Per-step directive passthrough. Every step carries the same + /// `DirectivePassthrough::inherit_from_env()` — the CD file, so a nested + /// `wt switch` can still move the parent shell. The source-dependent EXEC + /// passthrough this field used to select is gone (#3977); the shape stays + /// per-step because that is where the pipeline builds it. pub directives: DirectivePassthrough, } @@ -217,7 +207,8 @@ impl<'a> CommandContext<'a> { /// Resolve the template variables for one command invocation. /// /// The sole producer of [`TemplateContext`], which owns what happens to the -/// result: expansion, the JSON a hook child reads on stdin, the `-v` table. +/// result: expansion, the JSON a `wt step for-each` child reads on stdin, the +/// `-v` table. /// /// `scope` decides how much to resolve. [`VarScope::Referenced`] skips the git /// lookups behind vars the templates don't name (`var_commit` rev-parse, @@ -231,10 +222,19 @@ impl<'a> CommandContext<'a> { /// `referenced_vars_for_templates` over its command and trailing args. /// - **`All`** when something reads keys the `{{ }}` templates never mention. /// Either the child receives the whole context as JSON on stdin and may pull -/// keys out of it (e.g. via `jq`) — hook pipelines, `wt step for-each` — or -/// the command's output *is* the variable listing, which filtering would -/// narrow to what the body happens to reference: `wt step eval -v`, and the -/// hook pipeline's own `format_hook_variables` table. +/// keys out of it (e.g. via `jq`) — `wt step for-each` — or the command's +/// output *is* the variable listing, which filtering would narrow to what the +/// body happens to reference: `wt step eval -v`, and the hook pipeline's own +/// `format_hook_variables` table. +/// +/// The hook pipeline's reader is conditional, so its `All` in [`prepare_steps`] +/// buys nothing at default verbosity — `format_hook_variables` renders only at +/// `verbosity() >= 1`. A hook whose templates name nothing but `{{ branch }}` +/// still pays for `rev-parse --verify`, `primary_worktree()`, +/// `primary_remote()`, and `default_branch()`, whose first call per repo may +/// reach `git ls-remote`. Narrowing it wants a scope that varies with +/// verbosity, or a separate entry point for the paths that list or preview; +/// neither belongs in the change that removed the JSON reader. /// /// The template-preview paths (`render_hook_commands`, `wt config alias`) fit /// neither case and still pass `All`: they expand and print one line per @@ -530,7 +530,6 @@ fn run_concurrent_group( }) .collect(); - let context_jsons: Vec = cmds.iter().map(PreparedCommand::context_json).collect(); let log_labels: Vec> = cmds .iter() .map(|cmd| fg_step.announce.log_label(cmd)) @@ -542,7 +541,6 @@ fn run_concurrent_group( label: labels[i], expanded: &expanded[i], working_dir: wt_path, - context_json: &context_jsons[i], log_label: log_labels[i].as_deref(), directives, scrub_git_discovery, @@ -585,15 +583,15 @@ fn run_one_command( }; announce_command(cmd, &fg_step.announce, &command_str); - // Hooks get a documented JSON context on stdin; aliases inherit stdin so - // interactive children (e.g. `wt switch`'s picker) keep their controlling - // terminal. Piping JSON into an interactive alias body steals the tty. - let stdin_json = fg_step.pipe_stdin.then(|| cmd.context_json()); + // Foreground steps inherit the parent's stdin so an interactive child keeps + // its controlling terminal — a `pre-*` hook can prompt (e.g. `gum confirm` + // before `mise trust`), and an alias body's `wt switch` picker keeps the + // tty. Nothing is ever piped in: template variables are how a hook reads + // its context, whatever form it runs in. let log_label = fg_step.announce.log_label(cmd); let result = execute_shell_command( wt_path, &command_str, - stdin_json.as_deref(), log_label.as_deref(), directives.clone(), fg_step.redirect_stdout_to_stderr, @@ -772,7 +770,7 @@ impl PreparedPipeline { /// and background) and the `wt hook show --expanded` listing come through here, /// so a context key added here reaches both with no second edit. /// -/// Each command freezes its context as JSON and keeps its raw template; +/// Each command freezes its context and keeps its raw template; /// rendering happens when the command runs, so semantic errors (undefined /// variable, filter failure) surface at the failing step. The returned /// [`PreparedPipeline`] makes the caller choose what an unparsable template @@ -787,7 +785,7 @@ pub fn prepare_steps( // Built once per pipeline — build_hook_context spawns git subprocesses. let mut base_context = build_hook_context(ctx, extra_vars, VarScope::All)?; - // hook_type is always available as a template variable and in JSON context + // hook_type is always available as a template variable base_context.insert("hook_type", hook_type.to_string()); // `{{ args }}` is always available in hook scope. Default to an empty // JSON sequence (rendered via ShellArgs rehydration) so templates can @@ -802,7 +800,7 @@ pub fn prepare_steps( } let steps = map_config_steps(command_config, |cmd| { - // hook_name is per-command: available as template variable and in JSON context + // hook_name is per-command, available as a template variable let mut cmd_context = base_context.clone(); if let Some(ref name) = cmd.name { cmd_context.insert("hook_name", name.clone()); diff --git a/src/commands/hooks.rs b/src/commands/hooks.rs index 03e9b3afc..6aba69653 100644 --- a/src/commands/hooks.rs +++ b/src/commands/hooks.rs @@ -572,12 +572,16 @@ fn spawn_hook_pipeline_quiet(repo: &Repository, pipeline: PendingPipeline) -> an /// Convert source-tagged steps into foreground steps with pipeline-kind policy. /// /// Shared between hook and alias dispatch. The `kind` argument supplies the -/// per-call-site policy (announce style, stdin handling, error wrapping) while -/// the `source` field on each step drives the per-step trust model -/// (`DirectivePassthrough`). +/// per-call-site policy (announce style, stdout redirection, error wrapping). +/// Every step gets the same `DirectivePassthrough::inherit_from_env()`; the +/// EXEC passthrough the `source` field used to select is gone (#3977). /// -/// Every foreground step inherits the CD directive so a nested switch can -/// still move the parent shell. +/// Foreground steps — hook and alias alike — inherit the parent's stdin so an +/// interactive child keeps the controlling terminal (a `pre-*` hook can prompt; +/// an alias body's `wt switch` picker works). The forms that can't be +/// interactive get a closed stdin rather than a substitute payload: concurrent +/// children (they'd race for the terminal) and detached (`post-*`) hooks (there +/// is none). Template variables carry the context in every form. pub(crate) fn sourced_steps_to_foreground( sourced_steps: Vec, kind: &PipelineKind, @@ -586,16 +590,13 @@ pub(crate) fn sourced_steps_to_foreground( .into_iter() .map(|sourced| { let directives = DirectivePassthrough::inherit_from_env(); - let (pipe_stdin, redirect_stdout_to_stderr, error_wrapper) = match kind { - PipelineKind::Hook { hook_type, .. } => { - (true, true, hook_error_wrapper(*hook_type)) - } - PipelineKind::Alias { name } => (false, false, alias_error_wrapper(name.clone())), + let (redirect_stdout_to_stderr, error_wrapper) = match kind { + PipelineKind::Hook { hook_type, .. } => (true, hook_error_wrapper(*hook_type)), + PipelineKind::Alias { name } => (false, alias_error_wrapper(name.clone())), }; ForegroundStep { step: sourced.step, announce: kind.clone(), - pipe_stdin, redirect_stdout_to_stderr, error_wrapper, directives, diff --git a/src/commands/process.rs b/src/commands/process.rs index 6eb50a47b..da106ab27 100644 --- a/src/commands/process.rs +++ b/src/commands/process.rs @@ -106,6 +106,10 @@ impl HookLog { /// Get the separator needed before closing brace in POSIX shell command grouping. /// Returns empty string if command already ends with newline or semicolon. +/// +/// Unix-only: the `{ …; } &` grouping it feeds is `spawn_detached_unix`'s +/// backgrounding wrapper, which the Windows spawn has no counterpart for. +#[cfg(unix)] fn posix_command_separator(command: &str) -> &'static str { if command.ends_with('\n') || command.ends_with(';') { "" @@ -114,25 +118,6 @@ fn posix_command_separator(command: &str) -> &'static str { } } -/// Build the POSIX-shell payload for a detached spawn that pipes optional -/// `context_json` into `command`'s stdin. With a JSON context, wraps the -/// command in a `printf '%s' '…' | { …; }` group so the inner command receives -/// the JSON verbatim and pipeline parsing isn't perturbed by `&&`/`||` inside -/// `command`. Without one, returns `command` unchanged. Shared by the Unix -/// path and the Windows Git-Bash branch — both feed POSIX shells, so the JSON -/// is POSIX-single-quote-escaped regardless of host. -fn build_printf_pipe_command(command: &str, context_json: Option<&str>) -> String { - match context_json { - Some(json) => format!( - "printf '%s' {} | {{ {}{} }}", - shell_escape::unix::escape(json.into()), - command, - posix_command_separator(command) - ), - None => command.to_string(), - } -} - /// Create the log directory and file for a detached process. /// /// Returns `(log_path, log_file)`. Shared by `spawn_detached` and @@ -185,7 +170,6 @@ pub fn spawn_detached( command: &str, branch: &str, hook_log: &HookLog, - context_json: Option<&str>, ) -> anyhow::Result { let (log_path, log_file) = create_detach_log(repo, branch, hook_log)?; @@ -199,12 +183,12 @@ pub fn spawn_detached( #[cfg(unix)] { let low_priority = matches!(hook_log, HookLog::Internal(_) | HookLog::Shared(_)); - spawn_detached_unix(worktree_path, command, log_file, context_json, low_priority)?; + spawn_detached_unix(worktree_path, command, log_file, low_priority)?; } #[cfg(windows)] { - spawn_detached_windows(worktree_path, command, log_file, context_json)?; + spawn_detached_windows(worktree_path, command, log_file)?; } Ok(log_path) @@ -215,22 +199,15 @@ fn spawn_detached_unix( worktree_path: &Path, command: &str, log_file: fs::File, - context_json: Option<&str>, low_priority: bool, ) -> anyhow::Result<()> { use std::os::unix::process::CommandExt; - let full_command = build_printf_pipe_command(command, context_json); - // Wrap in braces so `&` backgrounds the entire compound command. // Without braces, `cmd1 && cmd2; cmd3 &` parses as two statements: // `cmd1 && cmd2` (foreground) then `cmd3 &` (background) — the semicolon // has lower precedence than `&`, so only the last segment is backgrounded. - let shell_cmd = format!( - "{{ {}{} }} &", - full_command, - posix_command_separator(&full_command) - ); + let shell_cmd = format!("{{ {}{} }} &", command, posix_command_separator(command)); // Detachment via process_group(0): puts the spawned shell in its own process group. // When the controlling PTY closes, SIGHUP is sent to the foreground process group. @@ -268,7 +245,6 @@ fn spawn_detached_windows( worktree_path: &Path, command: &str, log_file: fs::File, - context_json: Option<&str>, ) -> anyhow::Result<()> { use std::os::windows::process::CommandExt; use worktrunk::shell_exec::ShellConfig; @@ -280,28 +256,9 @@ fn spawn_detached_windows( let shell = ShellConfig::get()?; - // Build the command based on shell type - let mut cmd = if shell.is_posix { - // Git Bash available - use same syntax as Unix - let full_command = build_printf_pipe_command(command, context_json); - shell.command(&full_command) - } else { - // PowerShell fallback - let full_command = match context_json { - Some(json) => { - // PowerShell single-quote escaping: - // - Single quotes prevent variable expansion ($) and are literal - // - Backticks are literal in single quotes (NOT escape characters) - // - Only single quotes need doubling (`'` → `''`) - // See: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules - let escaped_json = json.replace('\'', "''"); - // Pipe JSON to the command via PowerShell script block - format!("'{}' | & {{ {} }}", escaped_json, command) - } - None => command.to_string(), - }; - shell.command(&full_command) - }; + // Git Bash and the PowerShell fallback both run the command as written — + // nothing is piped in, so neither needs a wrapper around it. + let mut cmd = shell.command(command); cmd.current_dir(worktree_path) .stdin(Stdio::null()) @@ -538,7 +495,6 @@ pub fn sweep_stale_trash(repo: &Repository) { &command, "", &HookLog::Shared(InternalOp::TrashSweep), - None, ) { tracing::debug!(error = %e, "Failed to spawn stale trash sweep: {e}"); } @@ -851,6 +807,7 @@ mod tests { } #[test] + #[cfg(unix)] fn test_posix_command_separator() { // Commands ending with newline don't need separator assert_eq!(posix_command_separator("echo hello\n"), ""); @@ -910,39 +867,6 @@ mod tests { assert_snapshot!(build_remove_command_staged(&special_path, &special_original, true), @"sleep 1 && rmdir -- '/tmp/test worktree' 2>/dev/null; rm -rf -- '/tmp/repo/.git/wt/trash/test worktree-123'"); } - #[test] - fn test_build_printf_pipe_command() { - // No JSON context: command passes through unchanged. - assert_snapshot!( - build_printf_pipe_command("echo hi", None), - @"echo hi" - ); - - // With JSON: command wrapped in a printf-pipe group. The JSON is - // POSIX-single-quote-escaped, the closing `}` is preceded by `;` - // because the command doesn't already end with one. - assert_snapshot!( - build_printf_pipe_command("jq .", Some(r#"{"branch":"main"}"#)), - @r#"printf '%s' '{"branch":"main"}' | { jq .; }"# - ); - - // Command that already ends in a semicolon: separator suppressed. - assert_snapshot!( - build_printf_pipe_command("jq .;", Some(r#"{"k":"v"}"#)), - @r#"printf '%s' '{"k":"v"}' | { jq .; }"# - ); - - // JSON containing characters POSIX shells treat specially must end up - // inside single quotes so command substitution doesn't fire inside the - // detached `sh -c` wrapping the spawn. The embedded single quote uses - // the standard `'\''` idiom. Regression guard for - // shell_escape::escape vs unix::escape on Windows. - assert_snapshot!( - build_printf_pipe_command("jq .", Some(r#"{"x":"$(echo pwned)","y":"a'b"}"#)), - @r#"printf '%s' '{"x":"$(echo pwned)","y":"a'\''b"}' | { jq .; }"# - ); - } - #[test] fn test_build_trash_sweep_command() { // Empty list still produces a well-formed command — the caller diff --git a/src/commands/run_pipeline.rs b/src/commands/run_pipeline.rs index 79f8e9814..aff41d693 100644 --- a/src/commands/run_pipeline.rs +++ b/src/commands/run_pipeline.rs @@ -31,9 +31,11 @@ //! git config, so order matters for `vars.*`), so a later command's expansion //! can run after an earlier command's child has already started. //! -//! **Stdin**: every child receives its prepared context as JSON on stdin, -//! matching the foreground hook convention. Commands that don't read stdin -//! ignore it. +//! **Stdin**: every child gets a closed stdin — this runner is detached, so +//! there is no terminal to hand over and nothing to read. The foreground path +//! inherits wt's stdin instead, so a step there can prompt (see +//! `execute_shell_command` in `output/handlers.rs`). Every template variable +//! reaches a step either way, through `{{ }}` expansion. //! //! ## Template freshness //! @@ -122,9 +124,8 @@ pub fn run_pipeline() -> anyhow::Result<()> { let log_file = create_command_log(&spec, &log_dir, &log_name)?; let expanded = expand_shell_template(&cmd.template, &cmd.context, &repo, &cmd.template_name)?; - let step_json = cmd.context_json(); let (mut child, mut trace) = - spawn_shell_command(&expanded, &spec.worktree_path, &step_json, log_file)?; + spawn_shell_command(&expanded, &spec.worktree_path, log_file)?; let status = wait_resolving(&mut child, &mut trace, &expanded)?; if !status.success() { return Err(failure_error( @@ -143,16 +144,18 @@ pub fn run_pipeline() -> anyhow::Result<()> { Ok(()) } -/// Spawn a shell command with context JSON piped to stdin. +/// Spawn a shell command with its output redirected to a log file. /// /// Uses `ShellConfig` for portable shell detection (Git Bash on Windows, /// `sh` on Unix). stdout/stderr are redirected to `log_file` so each /// command gets its own log. Returns the `Child` so the caller controls /// when to wait. +/// +/// Stdin is closed: a detached step has no terminal, so a read returns EOF +/// rather than blocking on one that isn't there. fn spawn_shell_command( expanded: &str, worktree_path: &Path, - context_json: &str, log_file: fs::File, ) -> anyhow::Result<(Child, CommandTrace)> { let shell = ShellConfig::get()?; @@ -160,21 +163,19 @@ fn spawn_shell_command( .try_clone() .context("failed to clone log file handle")?; // Start the trace just before spawning; the caller resolves it once the - // child is waited on (see `wait_resolving`). The step is fed its own - // `context_json` on stdin, so mark it stdin-reading — the same command - // across worktrees isn't a duplicate (different per-worktree input). - let mut trace = CommandTrace::new(None, expanded).reads_stdin(true); + // child is waited on (see `wait_resolving`). + let mut trace = CommandTrace::new(None, expanded); let mut command = shell.command(expanded); command .current_dir(worktree_path) - .stdin(Stdio::piped()) + .stdin(Stdio::null()) .stdout(Stdio::from(log_file)) .stderr(Stdio::from(log_err)); // Background hooks, like foreground ones, discover their repo from the // worktree cwd, not an inherited GIT_DIR/GIT_WORK_TREE (issue #3373). This // runner only ever executes hook pipelines, so the scrub is unconditional. scrub_git_discovery_env_vars(&mut command); - let mut child = match command.spawn() { + let child = match command.spawn() { Ok(child) => child, Err(e) => { trace.fail(&e); @@ -182,13 +183,6 @@ fn spawn_shell_command( } }; - // Write context JSON to stdin, then drop to close the pipe. - if let Some(mut stdin) = child.stdin.take() { - use std::io::Write; - // Ignore BrokenPipe — child may exit or close stdin early. - let _ = stdin.write_all(context_json.as_bytes()); - } - Ok((child, trace)) } @@ -244,9 +238,8 @@ fn run_concurrent_group( let log_file = create_command_log(spec, log_dir, &log_name)?; let expanded = expand_shell_template(&cmd.template, &cmd.context, repo, &cmd.template_name)?; - let cmd_json = cmd.context_json(); let (mut child, mut trace) = - spawn_shell_command(&expanded, &spec.worktree_path, &cmd_json, log_file)?; + spawn_shell_command(&expanded, &spec.worktree_path, log_file)?; *cmd_index += 1; if serial { diff --git a/src/config/expansion.rs b/src/config/expansion.rs index 0843909e8..753bf6e8e 100644 --- a/src/config/expansion.rs +++ b/src/config/expansion.rs @@ -79,7 +79,7 @@ pub fn base_vars() -> Vec<&'static str> { /// Reserved context key carrying a JSON-encoded `Vec` of positional /// CLI args forwarded to an alias. The key flows through -/// [`TemplateContext`]'s flat string map — stable for stdin JSON — and +/// [`TemplateContext`]'s flat string map — stable for JSON round-trips — and /// [`expand_template`] rehydrates it as a `ShellArgs` object so bare /// `{{ args }}` renders as a space-joined, shell-escaped string while /// indexing, iteration, and `length` behave like a sequence. @@ -109,14 +109,14 @@ pub const LIST_COLUMN_VARS: &[&str] = &["branch", "worktree_path", "worktree_nam /// The resolved template variables for one command invocation. /// /// Wraps the map `build_hook_context` produces and owns every operation on -/// it: expansion, the JSON a hook child reads on stdin, and the `-v` variables -/// table. Callers hold this rather than a bare map so the borrow +/// it: expansion, the JSON a `wt step for-each` child reads on stdin, and the +/// `-v` variables table. Callers hold this rather than a bare map so the borrow /// [`expand_template`] needs, the `serde_json` call, and the `(unset)` / /// `(unused)` rendering each have one home. /// -/// Serializes transparently, so the JSON a child reads and the pipeline spec a -/// background runner deserializes are both the flat `{"branch": "…", …}` -/// object the [`ALIAS_ARGS_KEY`] contract describes. +/// Serializes transparently, so the JSON a for-each child reads and the +/// pipeline spec a background runner deserializes are both the flat +/// `{"branch": "…", …}` object the [`ALIAS_ARGS_KEY`] contract describes. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(transparent)] pub struct TemplateContext(HashMap); @@ -172,7 +172,7 @@ impl TemplateContext { expand_template_with(template, &vars, escape_mode, repo, name, vars_mode) } - /// The JSON form piped to a child's stdin. + /// The JSON form piped to a `wt step for-each` child's stdin. pub fn to_json(&self) -> String { serde_json::to_string(&self.0) .expect("HashMap serialization should never fail") @@ -189,8 +189,9 @@ pub enum VarScope<'a> { /// looked up. The set comes from `referenced_vars_for_config` or /// [`referenced_vars_for_templates`]. Referenced(&'a BTreeSet), - /// Something reads keys the templates never mention: a child consuming - /// the JSON on stdin, or a command whose output is the variable listing. + /// Something reads keys the templates never mention: a `wt step for-each` + /// child consuming the JSON on stdin, or a command whose output is the + /// variable listing. All, } diff --git a/src/output/concurrent.rs b/src/output/concurrent.rs index f2c5e5011..a657a2473 100644 --- a/src/output/concurrent.rs +++ b/src/output/concurrent.rs @@ -17,10 +17,10 @@ //! ## Execution model //! //! For each command: -//! 1. Spawn a shell child with stdout+stderr piped and (on Unix) its own -//! process group so SIGINT/SIGTERM can be delivered to the whole tree. -//! 2. Pipe `context_json` to stdin if provided, then close. -//! 3. Launch two reader threads that read lines and send labeled lines on +//! 1. Spawn a shell child with stdout+stderr piped, stdin closed, and (on +//! Unix) its own process group so SIGINT/SIGTERM can be delivered to the +//! whole tree. +//! 2. Launch two reader threads that read lines and send labeled lines on //! a shared channel. A single consumer writes to stderr — one writer //! preserves line atomicity so readers never mix bytes mid-line. //! @@ -63,13 +63,11 @@ pub struct ConcurrentCommand<'a> { pub expanded: &'a str, /// Child's working directory. pub working_dir: &'a Path, - /// JSON blob written to the child's stdin and closed. Callers that have - /// no context to pass should supply `"{}"`. - pub context_json: &'a str, /// Optional label for `commands.jsonl` tracing. pub log_label: Option<&'a str>, /// Directive file env vars to pass through to the child. See - /// `DirectivePassthrough` for the trust model (CD passthrough, EXEC scrub). + /// `DirectivePassthrough`: the CD directive file is the only one passed + /// through — every child scrubs the rest. pub directives: &'a DirectivePassthrough, /// Scrub inherited git-discovery vars (`GIT_DIR`/`GIT_WORK_TREE`/…) from the /// child. `true` for hooks (they operate on the worktree wt targets), `false` @@ -288,10 +286,14 @@ fn spawn_child( index: usize, cmd: &ConcurrentCommand<'_>, ) -> anyhow::Result { + // Each child gets a closed stdin: it can't have the terminal (siblings run + // at the same time and would race for it), and each runs in its own process + // group, where a read from the controlling terminal earns SIGTTIN and stops + // the child rather than returning. A closed stdin reads EOF immediately. let mut command = shell.command(cmd.expanded); command .current_dir(cmd.working_dir) - .stdin(Stdio::piped()) + .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()); @@ -323,10 +325,8 @@ fn spawn_child( // Start the trace just before spawning so its duration brackets the real // spawn → wait span (the child keeps running while we drain its output). - // Each child is fed its own `context_json` on stdin, so mark it stdin-reading - // — the same command across worktrees isn't a duplicate (different input). - let mut trace = CommandTrace::new(None, cmd.expanded).reads_stdin(true); - let mut child = match command.spawn() { + let mut trace = CommandTrace::new(None, cmd.expanded); + let child = match command.spawn() { Ok(child) => child, Err(e) => { trace.fail(&e); @@ -335,11 +335,6 @@ fn spawn_child( } }; - if let Some(mut stdin) = child.stdin.take() { - // Ignore BrokenPipe — child may exit or close stdin early. - let _ = stdin.write_all(cmd.context_json.as_bytes()); - } - Ok(SpawnedChild { child, cmd_str: cmd.expanded.to_string(), @@ -494,7 +489,6 @@ mod tests { label, expanded: script, working_dir: &wd, - context_json: "{}", log_label, directives, scrub_git_discovery: false, diff --git a/src/output/handlers.rs b/src/output/handlers.rs index 8ac919083..ee582d67a 100644 --- a/src/output/handlers.rs +++ b/src/output/handlers.rs @@ -180,7 +180,6 @@ fn spawn_background_removal( &remove_command, log_label, &HookLog::Internal(InternalOp::Remove), - None, )?; } Ok(fate) @@ -2042,6 +2041,22 @@ fn remove_removed_worktree_silently( /// SIGINT/SIGTERM forwarding to child process group, ANSI reset before child /// runs, `Cmd` tracing/logging, and CD directive control. /// +/// ## Stdin +/// +/// The child always inherits the parent's stdin, so an interactive body keeps +/// the controlling terminal — a `pre-*` hook can `gum confirm`, an alias body's +/// `wt switch` picker can drive `/dev/tty`. `inherit_stdin()` also keeps the +/// child in wt's process group, which is what makes its `tcsetattr` on +/// `/dev/tty` succeed; see that method's docs for the SIGTTOU rationale, and +/// the "Process groups and signal handling" module docs in +/// [`worktrunk::shell_exec`] for what a shared pgroup costs on teardown. +/// +/// Nothing is ever written to that stdin — a hook reads its context through +/// template variables, whatever form it runs in. The two forms that can't be +/// interactive close stdin instead, in their own spawn paths: concurrent groups +/// in `output/concurrent.rs`, detached `post-*` pipelines in +/// `commands/run_pipeline.rs`. +/// /// ## Directive files /// /// `directives` controls whether the child can write shell-integration @@ -2084,7 +2099,6 @@ fn remove_removed_worktree_silently( pub fn execute_shell_command( working_dir: &std::path::Path, command: &str, - stdin_content: Option<&str>, command_log_label: Option<&str>, directives: DirectivePassthrough, redirect_stdout_to_stderr: bool, @@ -2118,16 +2132,10 @@ pub fn execute_shell_command( cmd = cmd.external(label); } - if let Some(content) = stdin_content { - cmd = cmd.stdin_bytes(content); - } else { - // Inherit the parent's stdin so interactive children (e.g. TUI - // pickers) keep their controlling terminal. `inherit_stdin()` also - // keeps the child in the parent's process group so `tcsetattr` on - // `/dev/tty` succeeds — see the method's doc comment for the - // SIGTTOU rationale. - cmd = cmd.inherit_stdin(); - } + // Inherit the parent's stdin so interactive children (e.g. TUI pickers, + // a `gum confirm` in a hook body) keep their controlling terminal — see + // the "Stdin" section of this function's docs. + cmd = cmd.inherit_stdin(); if let Some(path) = directives.cd_file { cmd = cmd.directive_cd_file(path); diff --git a/src/shell_exec.rs b/src/shell_exec.rs index 1a6e44a7e..b5729e542 100644 --- a/src/shell_exec.rs +++ b/src/shell_exec.rs @@ -16,8 +16,8 @@ //! - **Isolated** (`forward_signals()` alone): the child gets its own process //! group via `process_group(0)`. A signal_hook listener catches SIGINT/ //! SIGTERM in wt and `killpg`s the child group with SIGINT→SIGTERM→SIGKILL -//! escalation. Used for non-interactive children that may fork further -//! subprocesses (hook pipelines, alias steps that read from stdin) — `killpg` +//! escalation. Used for children wt runs on its own behalf across worktrees +//! (`wt step for-each`), which may fork further subprocesses — `killpg` //! reaches the whole subtree, which a shared-pgroup approach cannot. //! //! - **Shared-tty** (`forward_signals().inherit_stdin()`): the child stays in @@ -25,8 +25,13 @@ //! without the kernel raising SIGTTOU. Tty-initiated signals (Ctrl-C, hangup) //! reach the child via the kernel's foreground-pgroup broadcast; the listener //! additionally delivers externally-targeted signals (e.g. `kill -TERM -//! `) to the child by PID, single-shot. Used for interactive TUIs -//! (skim picker, pagers, `$EDITOR`). +//! `) to the child by PID, single-shot. Used for every `Single` +//! foreground step of a hook or alias pipeline, which inherits wt's stdin so +//! the step can prompt, and for the program `wt switch --execute` launches +//! (`execute_command` in `output/global.rs`). Such a child's own subtree is +//! therefore not reachable by `killpg`: an externally-targeted signal +//! reaches the child by PID and stops there, while Ctrl-C still reaches the +//! whole subtree through the kernel's broadcast. //! //! In both cases the listener still records `seen_signal`, so a signal-derived //! exit surfaces as `WorktrunkError::ChildProcessExited { signal: Some(_) }` — diff --git a/src/testing/mod.rs b/src/testing/mod.rs index ef0218ef5..b55bad842 100644 --- a/src/testing/mod.rs +++ b/src/testing/mod.rs @@ -3325,30 +3325,6 @@ pub fn wait_for_file_lines(path: &Path, expected_lines: usize) { ); } -/// Wait for a file to contain valid JSON, polling with exponential backoff. -/// Use when a background process writes JSON that may be partially written. -pub fn wait_for_valid_json(path: &Path) -> serde_json::Value { - let start = std::time::Instant::now(); - let mut attempt = 0; - let mut last_error = String::new(); - while start.elapsed() < BG_TIMEOUT { - if let Ok(content) = std::fs::read_to_string(path) { - match serde_json::from_str(&content) { - Ok(json) => return json, - Err(e) => last_error = format!("{e} (content: {content})"), - } - } - exponential_sleep(attempt); - attempt += 1; - } - panic!( - "File did not contain valid JSON within {:?}: {}\nLast error: {}", - BG_TIMEOUT, - path.display(), - last_error - ); -} - /// Poll until a condition is met, with exponential backoff. /// /// Use this instead of fixed sleeps for any condition that may take time to become true. diff --git a/tests/integration_tests/post_start_commands.rs b/tests/integration_tests/post_start_commands.rs index 262d5fa2d..aafb32f3e 100644 --- a/tests/integration_tests/post_start_commands.rs +++ b/tests/integration_tests/post_start_commands.rs @@ -2,7 +2,6 @@ use crate::common::{ SLEEP_FOR_ABSENCE_CHECK, TestRepo, make_snapshot_cmd, make_snapshot_cmd_with_global_flags, repo, repo_with_remote, resolve_git_common_dir, set_temp_home_env, setup_snapshot_settings, wait_for_file, wait_for_file_content, wait_for_file_count, wait_for_file_lines, - wait_for_valid_json, }; use insta::assert_snapshot; use insta_cmd::assert_cmd_snapshot; @@ -543,31 +542,46 @@ approved-commands = [ } #[rstest] -fn test_pre_start_json_stdin(repo: TestRepo) { +fn test_pre_start_inherits_stdin(repo: TestRepo) { use crate::common::wt_command; + use std::io::Write; + use std::process::Stdio; - // Create project config with a command that reads JSON from stdin - // Use cat to capture stdin to a file - repo.write_project_config(r#"pre-start = "cat > context.json""#); + // Foreground (`pre-*`) hooks inherit the parent's stdin — same as aliases — + // so an interactive child keeps the controlling terminal. Capture whatever + // the hook sees on stdin to a file and verify it's the parent's raw stdin. + repo.write_project_config(r#"pre-start = "cat > captured.txt""#); repo.commit("Add config"); // Pre-approve the command repo.write_test_approvals( r#"[projects."../origin"] -approved-commands = ["cat > context.json"] +approved-commands = ["cat > captured.txt"] "#, ); - // Create worktree - this should pipe JSON to the hook's stdin + // Create worktree, piping a sentinel to wt's stdin. The pre-start hook + // inherits that stdin, so `cat` captures the sentinel verbatim. let temp_home = TempDir::new().unwrap(); let mut cmd = wt_command(); - cmd.args(["switch", "--create", "feature-json"]) + cmd.args(["switch", "--create", "feature-stdin"]) .current_dir(repo.root_path()) .env("WORKTRUNK_CONFIG_PATH", repo.test_config_path()) - .env("WORKTRUNK_APPROVALS_PATH", repo.test_approvals_path()); + .env("WORKTRUNK_APPROVALS_PATH", repo.test_approvals_path()) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); set_temp_home_env(&mut cmd, temp_home.path()); - let output = cmd.output().expect("failed to run wt switch"); + + let mut child = cmd.spawn().expect("failed to spawn wt switch"); + child + .stdin + .take() + .expect("stdin piped") + .write_all(b"sentinel-from-parent-stdin\n") + .expect("failed to write to wt stdin"); + let output = child.wait_with_output().expect("failed to run wt switch"); assert!( output.status.success(), @@ -575,56 +589,33 @@ approved-commands = ["cat > context.json"] String::from_utf8_lossy(&output.stderr) ); - // Find the worktree and read the JSON - let worktree_path = repo.root_path().parent().unwrap().join("repo.feature-json"); - let json_file = worktree_path.join("context.json"); + let worktree_path = repo + .root_path() + .parent() + .unwrap() + .join("repo.feature-stdin"); + let captured = worktree_path.join("captured.txt"); assert!( - json_file.exists(), - "context.json should have been created from stdin" + captured.exists(), + "captured.txt should have been created from the inherited stdin" ); - let contents = fs::read_to_string(&json_file).unwrap(); - - // Parse and verify the JSON contains expected fields - let json: serde_json::Value = serde_json::from_str(&contents) - .unwrap_or_else(|e| panic!("Should be valid JSON: {}\nContents: {}", e, contents)); - - assert!( - json.get("repo").is_some(), - "JSON should contain 'repo' field" - ); - assert!( - json.get("branch").is_some(), - "JSON should contain 'branch' field" - ); - assert_eq!( - json["branch"].as_str(), - Some("feature-json"), - "Branch should be sanitized (feature-json)" - ); - assert!( - json.get("worktree").is_some(), - "JSON should contain 'worktree' field" - ); - assert!( - json.get("repo_root").is_some(), - "JSON should contain 'repo_root' field" - ); + let contents = fs::read_to_string(&captured).unwrap(); assert_eq!( - json["hook_type"].as_str(), - Some("pre-start"), - "JSON should contain hook_type" + contents, "sentinel-from-parent-stdin\n", + "Foreground hook should receive the parent's raw stdin" ); } #[rstest] #[cfg(unix)] -fn test_post_start_script_reads_json(repo: TestRepo) { +fn test_post_start_script_reads_template_args(repo: TestRepo) { use crate::common::wt_command; use std::os::unix::fs::PermissionsExt; - // Create a scripts directory and a Python script that reads JSON from stdin + // Create a scripts directory and a Python script that reads its context + // from argv — the channel every hook has, whatever form it runs in. let scripts_dir = repo.root_path().join("scripts"); fs::create_dir_all(&scripts_dir).unwrap(); @@ -634,36 +625,38 @@ fn test_post_start_script_reads_json(repo: TestRepo) { // path, so shebang resolution can't piggyback on PATH directly. let python = which::which("python3").expect("python3 in PATH for test"); let script_body = r#" -import json import sys -ctx = json.load(sys.stdin) +repo, branch, hook_type, hook_name = sys.argv[1:5] with open('hook_output.txt', 'w') as f: - f.write(f"repo={ctx['repo']}\n") - f.write(f"branch={ctx['branch']}\n") - f.write(f"hook_type={ctx['hook_type']}\n") - f.write(f"hook_name={ctx.get('hook_name', 'unnamed')}\n") + f.write(f"repo={repo}\n") + f.write(f"branch={branch}\n") + f.write(f"hook_type={hook_type}\n") + f.write(f"hook_name={hook_name}\n") "#; let script_content = format!("#!{}{}", python.display(), script_body); let script_path = scripts_dir.join("setup.py"); fs::write(&script_path, script_content).unwrap(); fs::set_permissions(&script_path, fs::Permissions::from_mode(0o755)).unwrap(); - // Create project config that runs the script - repo.write_project_config( - r#"[pre-start] -setup = "./scripts/setup.py" -"#, - ); + // Create project config that runs the script. Background (`post-*`) hooks + // run detached with a closed stdin, so the template variables in the + // command line are how the script receives its context. + let command = "./scripts/setup.py {{ repo }} {{ branch }} {{ hook_type }} {{ hook_name }}"; + repo.write_project_config(&format!( + r#"[post-start] +setup = "{command}" +"# + )); repo.commit("Add setup script and config"); // Pre-approve the command - repo.write_test_approvals( + repo.write_test_approvals(&format!( r#"[projects."../origin"] -approved-commands = ["./scripts/setup.py"] -"#, - ); +approved-commands = ["{command}"] +"# + )); // Create worktree let temp_home = TempDir::new().unwrap(); @@ -681,18 +674,15 @@ approved-commands = ["./scripts/setup.py"] String::from_utf8_lossy(&output.stderr) ); - // Find the worktree and verify the script wrote the expected output + // Find the worktree and verify the script wrote the expected output. The + // hook is detached, so poll until it finishes writing. let worktree_path = repo .root_path() .parent() .unwrap() .join("repo.feature-script"); let output_file = worktree_path.join("hook_output.txt"); - - assert!( - output_file.exists(), - "Script should have created hook_output.txt" - ); + wait_for_file_content(&output_file); let contents = fs::read_to_string(&output_file).unwrap(); assert!( @@ -706,7 +696,7 @@ approved-commands = ["./scripts/setup.py"] contents ); assert!( - contents.contains("hook_type=pre-start"), + contents.contains("hook_type=post-start"), "Output should contain hook_type: {}", contents ); @@ -718,25 +708,28 @@ approved-commands = ["./scripts/setup.py"] } #[rstest] -fn test_post_start_json_stdin(repo: TestRepo) { +fn test_post_start_detached_hook_gets_no_stdin(repo: TestRepo) { use crate::common::wt_command; - // Create project config with a background command that reads JSON from stdin - repo.write_project_config(r#"post-start = "cat > context.json""#); + // A detached hook has no terminal to inherit and receives no piped payload, + // so a command that reads stdin sees EOF straight away. The `&& echo` marks + // completion, since the captured file itself is expected to stay empty. + let command = "cat > captured.txt && echo done > done.txt"; + repo.write_project_config(&format!(r#"post-start = "{command}""#)); repo.commit("Add config"); // Pre-approve the command - repo.write_test_approvals( + repo.write_test_approvals(&format!( r#"[projects."../origin"] -approved-commands = ["cat > context.json"] -"#, - ); +approved-commands = ["{command}"] +"# + )); // Create worktree let temp_home = TempDir::new().unwrap(); let mut cmd = wt_command(); - cmd.args(["switch", "--create", "bg-json"]) + cmd.args(["switch", "--create", "bg-stdin"]) .current_dir(repo.root_path()) .env("WORKTRUNK_CONFIG_PATH", repo.test_config_path()) .env("WORKTRUNK_APPROVALS_PATH", repo.test_approvals_path()); @@ -749,24 +742,14 @@ approved-commands = ["cat > context.json"] String::from_utf8_lossy(&output.stderr) ); - // Find the worktree and wait for valid JSON (polls until cat finishes writing) - let worktree_path = repo.root_path().parent().unwrap().join("repo.bg-json"); - let json_file = worktree_path.join("context.json"); - let json = wait_for_valid_json(&json_file); + // The hook is detached, so wait for its completion marker before reading. + let worktree_path = repo.root_path().parent().unwrap().join("repo.bg-stdin"); + wait_for_file_content(&worktree_path.join("done.txt")); + let captured = fs::read_to_string(worktree_path.join("captured.txt")).unwrap(); assert_eq!( - json["branch"].as_str(), - Some("bg-json"), - "Background hook should receive JSON with branch" - ); - assert!( - json.get("repo").is_some(), - "Background hook should receive JSON with repo" - ); - assert_eq!( - json["hook_type"].as_str(), - Some("post-start"), - "Background hook should receive hook_type" + captured, "", + "a detached hook should read EOF — no JSON context, no inherited stdin" ); } diff --git a/tests/integration_tests/step_alias.rs b/tests/integration_tests/step_alias.rs index a291df7b2..edb86cf6e 100644 --- a/tests/integration_tests/step_alias.rs +++ b/tests/integration_tests/step_alias.rs @@ -972,8 +972,9 @@ fn test_alias_runs_execute_directly(repo: TestRepo) { /// /// Regression test for #406: alias execution used to pipe the template /// context JSON into each child's stdin, which displaced the tty and broke -/// `stdin().is_terminal()` guards in interactive commands. Only hooks have a -/// documented JSON-on-stdin contract; aliases must leave stdin alone. +/// `stdin().is_terminal()` guards in interactive commands. Hooks no longer +/// pipe JSON either, so the `"branch"` assertion below pins the rule for +/// every foreground step rather than a hooks-vs-aliases distinction. #[rstest] fn test_alias_inherits_stdin(repo: TestRepo) { repo.write_test_config( @@ -1018,7 +1019,7 @@ echo-stdin = "cat" // "branch" key) to stdin, so `cat` would have echoed that instead. assert!( !combined.contains("\"branch\""), - "alias stdin should not receive the hook JSON context, \ + "no foreground step should receive a JSON context on stdin, \ got stdout={stdout:?} stderr={stderr:?}", ); } diff --git a/tests/integration_tests/user_hooks.rs b/tests/integration_tests/user_hooks.rs index dc41395aa..46d2cf361 100644 --- a/tests/integration_tests/user_hooks.rs +++ b/tests/integration_tests/user_hooks.rs @@ -1946,6 +1946,146 @@ fn test_standalone_hook_post_start_foreground(repo: TestRepo) { ); } +#[rstest] +fn test_standalone_hook_post_start_foreground_inherits_stdin(repo: TestRepo) { + use std::io::Write; + use std::process::Stdio; + + // `--foreground` routes a `post-*` hook through the single-step foreground + // path, which inherits wt's stdin — so the hook can prompt in the mode that + // exists to debug it. (The detached default reads EOF; see + // `test_post_start_detached_hook_gets_no_stdin`.) + repo.write_project_config(r#"post-start = "cat > captured.txt""#); + + let mut cmd = crate::common::wt_command(); + cmd.current_dir(repo.root_path()); + cmd.env("WORKTRUNK_CONFIG_PATH", repo.test_config_path()); + cmd.args(["hook", "post-start", "--yes", "--foreground"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cmd.spawn().expect("failed to spawn wt hook post-start"); + child + .stdin + .take() + .expect("stdin piped") + .write_all(b"sentinel-from-parent-stdin\n") + .expect("failed to write to wt stdin"); + let output = child.wait_with_output().expect("failed to run wt hook"); + + assert!( + output.status.success(), + "wt hook post-start --foreground should succeed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let captured = repo.root_path().join("captured.txt"); + assert!( + captured.exists(), + "captured.txt should have been created from the inherited stdin" + ); + + let contents = fs::read_to_string(&captured).unwrap(); + assert_eq!( + contents, "sentinel-from-parent-stdin\n", + "`--foreground` should hand the hook the parent's raw stdin" + ); +} + +#[rstest] +fn test_foreground_pipeline_steps_share_one_stdin(repo: TestRepo) { + use std::io::Write; + use std::process::Stdio; + + // Foreground steps run in order against wt's own stdin, so the first one + // to drain it to EOF leaves nothing behind — which is a property of the + // pipe this test constructs, not of a terminal, where each step could + // still prompt in turn. Steps accumulate across config files, so a user + // and a project hook of the same type reach this shape without an array. + repo.write_project_config(r#"post-start = ["cat > a.txt", "cat > b.txt"]"#); + + let mut cmd = crate::common::wt_command(); + cmd.current_dir(repo.root_path()); + cmd.env("WORKTRUNK_CONFIG_PATH", repo.test_config_path()); + cmd.args(["hook", "post-start", "--yes", "--foreground"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cmd.spawn().expect("failed to spawn wt hook post-start"); + child + .stdin + .take() + .expect("stdin piped") + .write_all(b"sentinel-from-parent-stdin\n") + .expect("failed to write to wt stdin"); + let output = child.wait_with_output().expect("failed to run wt hook"); + + assert!( + output.status.success(), + "wt hook post-start --foreground should succeed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + assert_eq!( + fs::read_to_string(repo.root_path().join("a.txt")).unwrap(), + "sentinel-from-parent-stdin\n", + "the first foreground step should read the parent's stdin" + ); + assert_eq!( + fs::read_to_string(repo.root_path().join("b.txt")).unwrap(), + "", + "the first step drained stdin, so the second should see EOF" + ); +} + +#[rstest] +fn test_standalone_hook_concurrent_group_gets_no_stdin_under_foreground(repo: TestRepo) { + use std::io::Write; + use std::process::Stdio; + + // A multi-key table parses as `HookStep::Concurrent`, whose children run at + // the same time and so can't share one terminal — each gets a closed stdin + // instead. This is the shape that silently takes the tty away from a + // `gum confirm`, so pin it rather than leaving it to the docs. + repo.write_project_config( + r#"[post-start] +a = "cat > cap_a.txt" +b = "true" +"#, + ); + + let mut cmd = crate::common::wt_command(); + cmd.current_dir(repo.root_path()); + cmd.env("WORKTRUNK_CONFIG_PATH", repo.test_config_path()); + cmd.args(["hook", "post-start", "--yes", "--foreground"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cmd.spawn().expect("failed to spawn wt hook post-start"); + child + .stdin + .take() + .expect("stdin piped") + .write_all(b"sentinel-from-parent-stdin\n") + .expect("failed to write to wt stdin"); + let output = child.wait_with_output().expect("failed to run wt hook"); + + assert!( + output.status.success(), + "wt hook post-start --foreground should succeed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let contents = fs::read_to_string(repo.root_path().join("cap_a.txt")).unwrap(); + assert_eq!( + contents, "", + "a concurrent child should read EOF — neither the parent's stdin nor a JSON context" + ); +} + #[rstest] fn test_standalone_hook_pre_commit(repo: TestRepo) { // Write project config with pre-commit hook @@ -3909,8 +4049,8 @@ fn test_user_post_start_pipeline_shell_escaping(repo: TestRepo) { #[rstest] fn test_user_post_start_pipeline_hook_name_per_step(repo: TestRepo) { // Each step in a pipeline should see its own hook_name, not the first step's name. - // Before the fix, step 2 would see step 1's hook_name because the shared pipeline - // context included hook_name from the first command's context_json. + // Before the fix, step 2 would see step 1's hook_name because a single shared + // pipeline context carried the first command's hook_name to every step. repo.write_test_config( r#"post-start = [ { step_one = "echo {{ hook_name }} > step_one_name.txt" },