feat(hooks): connect foreground hooks to the terminal

[worktrunk-bot]

Problem

Foreground hooks couldn't run interactively. Every foreground hook had the JSON context piped to its stdin, which stole the controlling terminal, so a hook like gum confirm 'trust this worktree?' && mise trust could never see a TTY. The issue asked for a way to gate a hook behind a confirmation — #3093.

Solution

Per the maintainer's call in the thread ("implement the same as we have with aliases, so a simple foreground hook gets connected to stdin"), a hook running in the foreground now inherits the parent's stdin exactly as aliases already do, so it keeps the controlling terminal and can prompt before continuing:

[pre-start]
trust = "gum confirm 'trust this worktree?' && mise trust"

The lever is the one aliases already used: sourced_steps_to_foreground hard-coded pipe_stdin = true for hooks and false for aliases. With both sides now inheriting stdin in the single-step foreground path, the flag was uniformly false — so rather than leave it as a vestigial always-false field, the pipe_stdin field is removed and the foreground path simply never pipes. execute_shell_command's stdin_content parameter went the same way once its only caller passed None.

The JSON context is unchanged for the two forms that can't be interactive anyway: concurrent groups (whose children would race for one terminal) and detached post-* hooks. Template variables ({{ branch }}, etc.) reach every hook regardless of form.

Scope is deliberately the pre-* half only; promoting post-* hooks out of the detached path is the separate request tracked in #3102.

Open design question

The maintainer's objection in the thread is unresolved: whether the JSON-vs-terminal split is worth the complication at all, versus removing the JSON context entirely. That decision is still open and this PR does not settle it.

The evidence that accumulated while getting the docs right is the most useful input to it. The rule is keyed on PreparedStep::Single vs Concurrent, a distinction the TOML surface never names, and stating it correctly took four cases — each verified by a run against this branch, and each found only after the previous wording shipped:

• A foreground hook that reads stdin now blocks on the terminal rather than receiving JSON and an immediate EOF. Piping a sentinel into wt switch --create with pre-start = "cat > captured.txt" writes the sentinel verbatim. An existing hook written against the documented json.load(sys.stdin) contract hangs at a terminal instead of failing fast.
• A second key in the table flips it to a concurrent group, which keeps its JSON — enough to take the terminal away from the gum confirm this feature exists for. Two keys is a concurrent group; two tables in two config files is not.
• --foreground crosses the type boundary, in the mode that exists to debug a hook: post-start = "cat > capture.txt" captures 0 bytes under wt hook post-start --yes --foreground < /dev/null, against 1102 bytes of JSON in the default detached run. A concurrent group under --foreground still gets JSON (1118 bytes).
• Foreground steps share one stdin, and steps accumulate across config files, so a user [pre-start] and a project [pre-start] form one pipeline. Under a pipe the first step to drain stdin leaves the rest at EOF (verified: first step gets the sentinel, second gets 0 bytes); under a PTY each step prompts in turn (verified: first-line, then second-line).

Testing

• test_pre_start_inherits_stdin (reworked from test_pre_start_json_stdin): pipes a sentinel to wt switch's stdin and asserts a pre-start hook captures the raw bytes verbatim, and that the JSON context is not piped.
• test_standalone_hook_post_start_foreground_inherits_stdin: pins --foreground's terminal handoff — the pre-existing --foreground tests check only synchronous completion and stdout capture, so neither would notice the stdin shape flipping back.
• test_standalone_hook_concurrent_group_keeps_json_under_foreground: pins the other half, that a multi-key table's children still parse JSON off stdin.
• test_foreground_pipeline_steps_share_one_stdin: pins the drain, under the pipe where it applies.
• test_post_start_script_reads_json was misnamed — it configured a [pre-start] hook despite living among the post-start tests. Repointed at [post-start] (with wait_for_file_content for the detached write) so it covers the JSON-on-stdin path that survives.
• wt hook pre-merge --yes (the full gate: 4647 tests, clippy, pre-commit, doc sync) passes on the current head.

Docs

## Interactive hooks and JSON context in the hook docs and the hooks-vs-aliases stdin row in extending.md state which forms get the terminal and which get JSON, plus a CHANGELOG entry. The module specs that described the old convention are repointed: shell_exec's process-group shapes (foreground steps moved from Isolated to Shared-tty, so their subtrees are no longer reachable by killpg), run_pipeline's stdin paragraph, and sourced_steps_to_foreground.

Closes #3093

This was written by Claude Code on behalf of max-sixty

[worktrunk-bot]

The core change is clean and the docs/tests match it. I traced the two paths the PR claims still receive the JSON context and both hold: concurrent groups build context_jsons and write them to each child's stdin (command_executor.rs → output/concurrent.rs), and the detached post-* pipeline writes context_json in run_pipeline.rs. The foreground single-step path (run_one_command) is the only one that flips to inheriting stdin, which is exactly the pre-* half the issue asked for. The misnamed test_post_start_script_reads_json repoint (it was configured with [pre-start] while living among the post-start tests) is a real correctness improvement — it now exercises the JSON-on-stdin path that actually survives, with wait_for_file_content for the detached write.

One thing that looks unintended: this PR adds .claude/settings.local.json containing {"permissions":{"defaultMode":"bypassPermissions",...},"skipDangerousModePermissionPrompt":true}. That's a local agent-sandbox settings file (the .local.json suffix is the convention for per-machine, gitignore'd settings), and the content is unrelated to the foreground-hooks feature. On main this file is tracked but empty; this PR fills it with bypass-permission flags. Worth dropping it from the commit (and likely adding .claude/settings.local.json to .gitignore) so the feature PR stays scoped to the hook change.

[worktrunk-bot]

Good catch on the stray .claude/settings.local.json. Confirmed and addressed in a9f7bc2: that bypass-permissions file was an agent-sandbox artifact unrelated to the foreground-hooks change. I removed it from tracking (git rm --cached) and added .claude/settings.local.json to .gitignore so future sessions don't re-stage it.

One small correction to the review note: on main the file isn't tracked at all (it's a brand-new file in this PR), rather than "tracked but empty" — but the disposition is the same. The feature PR is now scoped to just the hook change.

[max-sixty]

I'm really not sure about adding the complication around whether or not we pipe JSON in.

I think we should probably either remove it completely or not merge this — any votes either way?

[worktrunk-bot]

My vote: merge as-is, with "remove JSON entirely" as the clean fallback and "don't merge" as the weakest option. Reasoning below — the key fact is that the JSON context carries no data the templates don't.

The split isn't a new axis. The JSON-vs-stdin distinction this PR introduces maps exactly onto the foreground/background split that already structures hooks: pre-* run in the foreground, post-* are detached background processes. JSON-on-stdin is physically what stole the tty — the prompt needs the tty on the stdin fd, the JSON needs that same fd. They can't coexist on one descriptor, so for pre-* you must pick one, and the thread already picked interactive. post-* are detached and can never be interactive, so JSON there is free. So the rule is "stdin carries the tty for hooks that can be interactive, JSON for the ones that can't" — which is the only coherent assignment, not really a second thing to remember.

If you want strict uniformity anyway, "remove it completely" is strictly better than "don't merge" — because the JSON context loses nothing. It's built from the same HashMap<String, String> that feeds template expansion (command_executor.rs:209 — "Build hook context as a HashMap for JSON serialization and template expansion"; context_json() at :46 just serializes it). Every value is a flat string already reachable as {{ branch }} etc. So dropping JSON-on-stdin removes a delivery mechanism, not any data — the only thing it costs is hooks that prefer json.load(sys.stdin) over passing {{ }} as args (quoting-safety and programmatic iteration being the real niche). That collapses the model to one rule ("hooks inherit stdin; use templates for context") and still ships the #3093 interactivity. "Don't merge" is the worst of the three: it abandons the requested feature to preserve the JSON path that templates already subsume.

So my ranking: merge as-is ≥ remove JSON entirely ≫ don't merge.

Caveats if you go the "remove entirely" route

• It's a documented behavior break (hook.md § JSON context), so per the maturing-mode policy it'd want a deprecation note rather than a silent drop — though there's no config-format or CLI-flag change, only runtime stdin behavior.
• post-* are detached with stdin(Stdio::null()) today; removing JSON just leaves them on null stdin, which is fine.
• I can spin this variant up quickly if you'd prefer it — it's a smaller diff than this PR (delete the context_json plumbing + the post-* JSON test, swap the docs example to templates).

[worktrunk-bot]

Two consequences of the stdin change that the PR doesn't cover, plus one leftover. Nothing here re-argues the merge-as-is vs. remove-JSON question above — but the first finding is a third case that rule doesn't cover, surfaced while 8266ac4 was tightening it to cover the second.

wt hook post-* --foreground silently loses the JSON context. --foreground is the debugging mode for background hooks (--foreground Run in foreground (block until complete)), and it routes run_post_hook → run_hooks_foreground → run_one_command, so it now hands the hook the parent's stdin instead of the context. Verified against a build of this branch, same post-start = "cat > capture.txt" hook both times:

$ wt hook post-start --foreground --yes < /dev/null
capture.txt: 0 bytes

$ wt hook post-start --yes < /dev/null          # default: background
capture.txt: 634 bytes — {"target":"main","branch":"main","cwd":"…",…}

So a json.load(sys.stdin) hook reads empty under a redirect, and blocks on the terminal waiting for EOF when run from a tty — in the one mode that exists to debug it. The docs added here state the rule by hook type (pre-* vs post-* / concurrent), but the lever is the execution path, and --foreground crosses it. Either the flag keeps piping the context (it's the closest thing to "what the background run does", which is what makes it useful for debugging), or the documented rule needs a third case.

Foreground hooks changed signal shape, and shell_exec's spec still describes the old one. execute_shell_command's no-payload branch is inherit_stdin(), which sets share_parent_pgroup, so foreground hooks moved from the Isolated shape to Shared-tty. The "Process groups and signal handling" module doc in src/shell_exec.rs still lists them under Isolated — "Used for non-interactive children that may fork further subprocesses (hook pipelines, alias steps that read from stdin) — killpg reaches the whole subtree, which a shared-pgroup approach cannot" — and that last clause is exactly what foreground hooks give up: an externally-delivered kill -TERM <wt-pid> during a pre-merge hook is now forwarded single-shot to the hook shell's PID rather than killpg'd across its subtree, and there's no SIGINT → SIGTERM → SIGKILL escalation. Ctrl-C is unaffected (the kernel broadcasts to the shared foreground pgroup). Worth naming in the PR body, and the spec needs correcting either way.

execute_shell_command's stdin_content parameter is now vestigial. In src/output/handlers.rs, the single remaining caller passes a literal None, so the if let Some(content) = stdin_content { cmd = cmd.stdin_bytes(content) } branch is unreachable — the same argument the commit message makes for dropping pipe_stdin rather than leaving it uniformly false.

Smaller note, not this PR

prepare_steps resolves hook contexts with VarScope::All, justified by the JSON contract. A single-step pre-* hook at verbosity 0 no longer has a reader for the variables its templates don't name, and build_hook_context spawns git subprocesses on the wt switch --create path — alias.rs and switch.rs already narrow to VarScope::Referenced for that reason.

Happy to push the shell_exec spec correction and the stdin_content removal onto this branch if you'd rather have them here than as follow-ups.

[worktrunk-bot]

The two new commits close all three findings from the previous review — I re-verified each. stdin_content is gone with no dead code left behind (PreparedCommand::context_json still has a live caller at the concurrent fan-out in command_executor.rs), the --foreground case is now stated in the hook docs, and the shell_exec Isolated bullet is accurate: for_each.rs is the only remaining forward_signals()-without-inherit_stdin() call site in the tree. Nothing below re-opens the merge-as-is vs. remove-JSON question.

Two more spec-staleness findings of the same class the commits just fixed, plus a test gap.

run_pipeline.rs's module doc still asserts the convention this PR inverted. Its Execution-model section reads "Stdin: every child receives the spec's context as JSON on stdin, matching the foreground hook convention." Foreground hooks are now precisely the thing that doesn't match — the sentence was the cross-reference that made the background behavior legible, and it now points at the opposite rule. It's outside the diff, so no inline suggestion; the fix is to drop the clause or repoint it (e.g. "…as JSON on stdin — the foreground path inherits wt's stdin instead, see output/handlers.rs's execute_shell_command"). Same file, spawn_pipeline_command at the "the same command" comment, is fine as-is.

The Shared-tty bullet's example list doesn't name a real Cmd call site — inline suggestion below. The bullet the commit extended still leads with "Used for interactive TUIs (skim picker, pagers, $EDITOR)", and none of the three go through Cmd: skim is the in-process skim crate (skim::prelude in picker/preview_orchestrator.rs, no child at all), both pagers spawn a bare std::process::Command (help_pager.rs's pipe_through_pager builds ShellConfig::command(...), which returns std::process::Command; picker/pager.rs uses Command::new("sh")), and $EDITOR isn't spawned anywhere — the only match in src/ outside this doc line is an rc-file fixture string. Since inherit_stdin() has exactly one call site (execute_shell_command), foreground hook and alias steps aren't also Shared-tty — after this PR they're the only thing that is, which is a stronger and simpler statement than the one the doc makes.

The same suggestion tightens "every single foreground step", which reads as emphasis ("each and every") rather than as the PreparedStep::Single vs Concurrent distinction it means — and the wrong reading is the one that matters here, since a concurrent group's children get process_group(0) and their own stdin pipe in output/concurrent.rs, i.e. the other shape.

Nothing pins the newly documented --foreground behavior. The docs now commit to "a post-* hook invoked that way gets the terminal and no JSON", which is a behavior change from before this PR, and it's the case the review thread turned on. The existing wt hook post-start --yes --foreground tests in tests/integration_tests/user_hooks.rs assert only that the hook completed synchronously and that its stdout reached the command output — neither would notice the stdin shape flipping back. test_pre_start_inherits_stdin is the right template: same sentinel-through-Stdio::piped() shape, wt hook post-start --foreground instead of wt switch --create.

Smaller notes

• hooks.rs's new sourced_steps_to_foreground doc says the JSON reaches "concurrent and background (post-*) hooks". handlers.rs and run_one_command both say detached post-*, which is the qualifier that makes the sentence survive --foreground; worth matching the three.
• No CHANGELOG.md entry under ## Unreleased. Given this both closes #3093 and breaks the documented json.load(sys.stdin) contract for pre-*, it's the kind of entry the release pass is most likely to under-describe if it's derived from the commit subjects alone.

Happy to push any of these onto the branch — though given the open question above, they may be better held until that's settled.

[worktrunk-bot]

All four items are on the branch — three commits, pushed as f4e84d6.

• run_pipeline.rs's stale cross-reference — d69c43c. Dropped "matching the foreground hook convention" and repointed it at execute_shell_command, which is where the foreground path's stdin decision now lives.
• The Shared-tty bullet — same commit, suggestion applied verbatim.
• hooks.rs wording — same commit; sourced_steps_to_foreground now says "concurrent groups and detached (post-*) hooks", matching handlers.rs and run_one_command.
• --foreground test gap — a0f534c adds test_standalone_hook_post_start_foreground_inherits_stdin, built on test_pre_start_inherits_stdin's shape: a sentinel through Stdio::piped() into wt hook post-start --yes --foreground, with a post-start = "cat > captured.txt" hook, asserting the captured bytes equal the sentinel. It fails if the JSON context comes back on that path.
• Changelog — f4e84d6 adds the ## Unreleased entry, leading with the gum confirm case and naming the json.load(sys.stdin) break plus the two forms that keep the JSON.

Verified locally: the five hook-stdin tests pass (test_pre_start_inherits_stdin, test_post_start_json_stdin, test_standalone_hook_post_start{,_foreground,_foreground_inherits_stdin}), cargo fmt --check and cargo clippy --all-targets are clean.

None of this touches the open question above — @max-sixty, that's still the one thing gating this PR, and my vote stands as posted: merge as-is ≥ remove the JSON context entirely ≫ don't merge. If you pick "remove entirely", the changelog entry and the --foreground doc sentence are the two pieces that would need rewriting; the test and the spec corrections survive either way.

[worktrunk-bot]

Two findings on the three new commits. Neither touches the open merge-as-is vs. remove-JSON question.

--foreground does not hand a concurrent group the terminal, but the changelog and the hook docs say it does. The new ## Interactive hooks and JSON context section closes with "wt hook <type> --foreground runs a hook in the foreground whatever its type, so a post-* hook invoked that way gets the terminal and no JSON", and f4e84d6's changelog entry repeats it as "runs in the foreground whatever the type, so it gets the terminal too". Both are unconditional, and both are wrong for a table with two or more keys: map_to_step in src/config/commands.rs turns a multi-entry map into HookStep::Concurrent, run_post_hook's if foreground.unwrap_or(false) branch hands it to run_hooks_foreground, and execute_pipeline_foreground's PreparedStep::Concurrent arm routes it to run_concurrent_group — which still builds context_jsons and writes one per child. So wt hook post-start --foreground against a two-key [post-start] table gets the JSON, not the terminal. The paragraph does state the concurrent rule two sentences earlier, but "whatever its type" reads as the override, and this is the sentence a user consults when a --foreground debug run behaves unlike the real one. Inline suggestion on the changelog line; the same clause is in src/cli/mod.rs (the after_long_help primary) and its four rendered mirrors, so that half wants an edit plus a doc-sync regen rather than a one-click apply.

Nothing pins the concurrent half of the "JSON survives" claim. The design in both the changelog and the docs rests on two surviving forms — concurrent groups and detached post-* hooks. test_post_start_json_stdin covers the detached one, and a0f534c just closed the --foreground gap, but no test feeds a multi-key hook table and reads what lands on its children's stdin: outside src/output/concurrent.rs itself, context_json appears in the test tree only as a comment in user_hooks.rs (test_user_post_start_pipeline_hook_name_per_step asserts through {{ hook_name }} templates, not stdin), and the sole fixture is a context_json: "{}" in concurrent.rs's own unit test, which asserts nothing about delivery. That matters more after this PR than before it: concurrent is now the only foreground path still piping JSON, one match arm away in the same execute_pipeline_foreground loop as the arm this PR flipped, so a later change that simplifies run_concurrent_group the way this one simplified run_one_command passes green. test_standalone_hook_post_start_foreground_inherits_stdin is the template with the assertion inverted — a two-key [post-start] table (a = "cat > a.json", b = "cat > b.json"), a sentinel written to wt's stdin, then assert each file parses as JSON carrying its own hook_name and that neither contains the sentinel.

Happy to push both onto the branch if you'd rather have them here — though as before they may be better held until the design question above is settled.

[worktrunk-bot]

Both findings from the previous review are closed on 0dd61e8, verified: the --foreground clause now carves out concurrent groups in CHANGELOG.md, src/cli/mod.rs's after_long_help and all four rendered mirrors (grep for "A concurrent group keeps its JSON either way" hits all six, so test_docs_are_in_sync should be satisfied), and test_standalone_hook_concurrent_group_keeps_json_under_foreground pins the concurrent half — the serde_json::from_str on cap_a.txt fails if the sentinel arrives instead, so it pins both directions of the claim.

One new thing, and it bears on the open question above rather than being a wording nit.

The rule now needs a third case, and "lone" doesn't cover it. A serial pipeline is neither lone nor concurrent, and it lands on the terminal side. execute_pipeline_foreground matches on PreparedStep, and every Single arm — however many of them a pipeline has — goes to run_one_command, which since this PR has no stdin branch left at all: execute_shell_command is unconditionally inherit_stdin(). So post-start = ["cat > a.txt", "cat > b.txt"] under --foreground hands the terminal to both steps and JSON to neither, and the first cat drains wt's stdin so the second sees EOF. The commoner trigger isn't the array form — it's layering: a user [pre-start] and a project [pre-start] merge by appending steps (CommandConfig's "commands are appended, matching how hooks merge across config layers"), so two single-key tables in two different config files produce exactly this shape. Before 0dd61e8 the docs said "whatever its type", which was right for a serial pipeline and wrong for a concurrent group; "lone" fixes the concurrent half and opens this one. Inline suggestion on the changelog line; src/cli/mod.rs carries the same clause and wants an edit plus a doc-sync regen rather than a one-click apply.

Which is the part worth weighing on the merge question: stating this correctly takes three cases keyed on an internal distinction (PreparedStep::Single vs Concurrent) that nothing in the TOML surface names — a second key in one table is a concurrent group, a second table across two config files is not. I voted merge-as-is earlier; on this increment I'd shift toward "remove it completely". Dropping the JSON everywhere collapses the rule to one sentence with no cases and no way for a config edit to silently move a hook across it, at the cost of the json.load(sys.stdin) contract for post-* — which {{ }} substitution already covers for everything except a hook that wants the whole context as one object. Not my call, but the third case is new evidence and it points that way.

Trace

• src/commands/command_executor.rs, execute_pipeline_foreground: for fg_step in steps { match &fg_step.step { PreparedStep::Single(cmd) => run_one_command(...), PreparedStep::Concurrent(cmds) => run_concurrent_group(...) } } — the loop is over steps, so a serial pipeline hits the Single arm once per step.
• src/output/handlers.rs, execute_shell_command: cmd = cmd.inherit_stdin(); with no surrounding conditional after this PR.
• src/config/commands.rs, CommandConfig: "Pipeline: post-start = ["cmd", { a = "cmd1", b = "cmd2" }] → serial steps"; map_to_step returns Single for a one-entry map and Concurrent otherwise.
• append_aliases / merge_append: "On name collision, commands are appended (base first, then additions), matching how hooks merge across config layers."

Code trace, not a run — the path has no branches, and test_pre_start_inherits_stdin already exercises the single-step form of it.

[worktrunk-bot]

The serial-pipeline case from the previous review is closed on aef32b3 — the docs and changelog both state it, and test_foreground_pipeline_steps_share_one_stdin pins it in both directions (asserting b.txt is empty is what makes it a pin rather than just "the first step read something"). Nothing below reopens the merge-as-is vs. remove-JSON question.

One finding on the new prose.

The new sentence is right about pipes and wrong about terminals — and it's the terminal case it's phrased for. docs/content/hook.md (via src/cli/mod.rs's after_long_help, plus the three mirrors) now closes with "a step that reads it to EOF leaves nothing for the steps behind it — only one step in a pipeline can prompt", and CHANGELOG.md compresses that to "only the first to read it sees anything". The leading clause is exact; both summaries aren't. Every foreground step gets Stdio::inherit() of the same descriptor — execute_shell_command ends at cmd = cmd.inherit_stdin(); with no conditional left after this PR — and "EOF is permanent" is a property of a pipe, not of a tty: a read() on a terminal returns the line the user typed and leaves the descriptor readable. So gum confirm in step 1 and gum confirm in step 2 both prompt normally. That's the shape the paragraph is warning about, since the trigger it names — a user [pre-start] and a project [pre-start] merging into one pipeline — is two prompting hooks, not two cats, and a user who reads "only one step can prompt" will restructure config that already worked.

What holds is narrower: a step that drains stdin to EOF (cat, json.load(sys.stdin)) starves the steps behind it, and only when wt's stdin is a pipe or a file — which is exactly what the new test constructs (Stdio::piped() plus cat > a.txt). Under a terminal neither half of the summary is true.

Inline suggestion on the changelog line. src/cli/mod.rs carries the docs half, so that one wants an edit plus a doc-sync regen rather than a one-click apply — replacing the — only one step in a pipeline can prompt clause with something like "…leaves nothing for the steps behind it when wt's stdin is a pipe or a file; under a terminal each step can prompt in turn" keeps the warning the paragraph is there to give while dropping the part that isn't true of a tty.

Trace

• src/commands/command_executor.rs, execute_pipeline_foreground: for fg_step in steps { match &fg_step.step { PreparedStep::Single(cmd) => run_one_command(...), … } } — one Single arm per pipeline step, each reaching execute_shell_command.
• src/output/handlers.rs, execute_shell_command: cmd = cmd.inherit_stdin();, unconditional.
• src/shell_exec.rs, Cmd::inherit_stdin: self.stdin_cfg = Some(std::process::Stdio::inherit()); — the child receives wt's own stdin descriptor, so the pipe-vs-tty distinction is the kernel's, not wt's.

Code trace plus the POSIX read semantics, not a tty run — the existing pipe-backed tests already cover the half that is true, and the tty half isn't reachable from the integration suite.

[worktrunk-bot]

CI status on a521907, for the record: test (linux), test (macos), lint, check-docs, code-coverage, codecov/patch, and codecov/project all pass. test (windows) (required) plus affected tests (windows, advisory) and affected tests (macos, advisory) were still running when my poll budget ran out, so they're unverified from here — the change is prose only (the pipeline-stdin sentence in src/cli/mod.rs and its three generated mirrors, the changelog line, and a test comment), with no assertion or production code touched.