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

One rule, no exceptions by config shape: a hook running in the foreground inherits wt's stdin; a hook that can't hold a terminal reads EOF; context reaches every hook through template variables.

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

JSON-on-stdin is gone for hooks entirely. It was the only reason stdin depended on whether a hook was a single step, a serial pipeline, a concurrent group, or a --foreground invocation — and it carried no data the templates don't. A hook that parsed it takes what it needs as arguments instead:

[post-start]
setup = "python3 scripts/post-start-setup.py {{ branch }} {{ repo }}"

wt step for-each keeps its JSON context; that's a separate documented feature, not a hook.

What was removed

• PreparedCommand::context_json and TemplateContext's use as a stdin payload for hooks.
• ConcurrentCommand::context_json — a concurrent child now spawns with Stdio::null().
• run_pipeline's per-step JSON write — a detached step spawns with Stdio::null().
• spawn_detached's context_json parameter and build_printf_pipe_command. Both callers already passed None, so that whole POSIX/PowerShell escaping path was dead once the detached hook runner stopped being its notional user.
• reads_stdin(true) on both spawn paths: with nothing piped in, two runs of the same (command, context) pair really are duplicates, so wt config state logs profile's cache report can see them again.

One place the rule is enforced rather than followed

A concurrent group's children get a closed stdin, not the terminal. They run at the same time and can't share a prompt, and each runs in its own process group (that's what lets wt forward SIGINT to a whole subtree), where a read from the controlling terminal earns SIGTTIN and stops the child instead of returning — a hang where the old behavior gave EOF. Handing them the tty would need the concurrent runner's signal model rebuilt. So the observable rule is "foreground single/serial steps get the terminal, everything else reads EOF", and no form receives a payload.

Testing

• test_pre_start_inherits_stdin, test_standalone_hook_post_start_foreground_inherits_stdin, test_foreground_pipeline_steps_share_one_stdin — unchanged, they pin the interactivity this PR exists for.
• test_standalone_hook_concurrent_group_gets_no_stdin_under_foreground (was …_keeps_json_under_foreground) — a concurrent child's capture file is empty.
• test_post_start_detached_hook_gets_no_stdin (was test_post_start_json_stdin) — a detached cat > captured.txt writes zero bytes; an && echo done marker makes the empty file waitable.
• test_post_start_script_reads_template_args (was test_post_start_script_reads_json) — same detached-script coverage, context now arriving as argv.
• Locally: cargo clippy --all-targets --all-features clean, cargo test --lib --bins (2483) green, cargo test --test integration 1999/2000 green, cargo fmt --check clean, doc sync regenerated. The one failure, test_copy_ignored_preserves_file_executable_permissions, reproduces identically on the branch with these changes stashed — it's the CI sandbox's umask 0002 giving 0664 where the test expects 0644.

Docs

## Interactive hooks in the hook docs (src/cli/mod.rs, with the rendered mirrors regenerated), the hooks-vs-aliases stdin row in extending.md, and the CHANGELOG entry all state the single rule and the breaking removal. The module specs that described the old convention are repointed: execute_shell_command, run_pipeline's stdin paragraph, sourced_steps_to_foreground, output/concurrent.rs's execution model, and TemplateContext/VarScope, whose "JSON a child reads" now names wt step for-each as its one remaining consumer.

Follow-up, deliberately not in this PR

prepare_steps still resolves the hook context with VarScope::All, and removing the JSON reader removed the unconditional reason it was there. What's left is format_hook_variables, which renders only at verbosity() >= 1 — so at the default verbosity a hook pipeline resolves keys nothing reads, paying for the git lookups scope.wants(…) gates: rev-parse --verify, short_sha, primary_worktree(), primary_remote()/remote_url()/upstream(), and default_branch(), whose first call per repo may fall through to git ls-remote. A pre-start hook whose templates name nothing but {{ branch }} can therefore put a fresh clone on the wire.

That cost predates this PR and narrowing it isn't a one-liner — wt hook show --expanded and the -v table both still want All, so the fix is a scope that varies with verbosity or a second entry point for the listing/preview paths. Either is a behavior change in a hot path, unrelated to stdin, so it belongs in its own PR rather than in the change that removed the reader. The build_hook_context doc comment now names the cost and the two fix shapes where the scope is justified, instead of leaving the bullet implying a full-time reader.

Closes #3093

[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.

[max-sixty]

The later test and documentation work makes the tradeoff clear: let's take the simpler route and remove JSON-on-stdin for hooks entirely.

Foreground hooks should inherit stdin, detached hooks should keep null stdin, and hook context should come through the existing template variables. That gives the feature in #3093 one stable rule instead of making stdin depend on whether a hook becomes a single step, a serial pipeline, a concurrent group, or a --foreground invocation. Please keep the interactivity tests, remove the JSON delivery plumbing and JSON-specific tests, and describe the documented behavior change in the changelog.

This was written by Codex on behalf of max-sixty

[worktrunk-bot]

Done — JSON-on-stdin is gone for hooks entirely, pushed as cd5e135 (plus a merge of main, which the branch had drifted into conflict with; that's why the last push registered no test checks — GitHub can't build the merge ref for a conflicting PR, so only pull_request_target workflows ran).

The rule is now: a hook running in the foreground inherits wt's stdin, anything else reads EOF, and context arrives through template variables in every form. Removed with the delivery plumbing: PreparedCommand::context_json, ConcurrentCommand::context_json, run_pipeline's per-step JSON write, and spawn_detached's context_json parameter along with build_printf_pipe_command — both of that helper's callers already passed None, so its POSIX/PowerShell escaping was dead weight once the detached runner stopped being its notional user. Both spawn paths also drop reads_stdin(true), so identical (command, context) pairs count as duplicates again in the cache report. wt step for-each keeps its JSON; that's a separate documented feature.

One place I enforced the rule rather than following it literally: a concurrent group's children get a closed stdin, not the terminal. They run at once and can't share a prompt, and each runs in its own process group — which is what lets wt forward SIGINT to a whole subtree — so a read from the controlling terminal earns SIGTTIN and stops the child instead of returning. That would turn today's immediate EOF into a hang, and handing them the tty would mean rebuilding the concurrent runner's signal model. So the shape the docs state is "foreground single/serial steps get the terminal, everything else reads EOF"; no form receives a payload, which is the part that was making stdin depend on config shape. Happy to revisit if you'd rather they inherit and take the SIGTTIN risk.

Tests: the three interactivity tests are unchanged; the JSON-delivery tests are repointed at the new rule (test_standalone_hook_concurrent_group_gets_no_stdin_under_foreground, test_post_start_detached_hook_gets_no_stdin), and test_post_start_script_reads_template_args keeps the detached-script coverage with the context arriving as argv. The changelog entry now leads with the removal and shows the argv replacement for a json.load(sys.stdin) hook.

Local verification

On the merged tree: cargo clippy --all-targets --all-features clean, cargo fmt --check clean, cargo test --lib --bins 2497 green, cargo test --test integration 1997/1998 green, test_docs_are_in_sync regenerated the docs/content/ and skill mirrors.

The one integration failure is test_copy_ignored_preserves_file_executable_permissions (expects 0644, gets 0664). It reproduces identically with these changes stashed, on the same checkout — it's this sandbox's umask 0002, not the change.

[worktrunk-bot]

897fcc7 answers the open question the way you asked — JSON-on-stdin is gone outright rather than split by config shape. I re-derived the removal independently and it's complete: context_json survives only for wt step for-each, every execute_shell_command / spawn_detached call site is updated, and nothing is left dead behind it. The main merge is clean too — git diff main..cd5e135 is exactly this PR's own changes, and both conflict hunks kept main's HookLog::Internal / HookLog::Shared variants while dropping the None argument.

Two things, neither blocking.

The VarScope::All note I deferred two reviews ago is now this PR's. prepare_steps still resolves the hook context with VarScope::All, and the doc comment immediately above it — which this diff edits — used to justify that with "the child receives the whole context as JSON on stdin … hook pipelines, wt step for-each". The edit narrows the JSON case to wt step for-each alone and leaves hook pipelines resting on the other justification, format_hook_variables. But that table only renders at verbosity() >= 1 (print_background_variable_table sits behind if verbosity() >= 1 in hooks.rs), so at the default verbosity a hook pipeline now resolves All with no reader at all. The cost is real: build_hook_context's scope.wants(…) gates are what stand between a hook and rev-parse --verify, short_sha, primary_worktree(), primary_remote() / remote_url() / upstream() — and default_branch(), which per CLAUDE.md's Network Access rule may fall through to git ls-remote on its first call per repo. So a pre-start hook whose templates name nothing but {{ branch }} can put a fresh clone on the wire.

It's pre-existing cost rather than a regression, and narrowing it isn't free — wt hook show --expanded and the -v table both still want All, so the fix is either a verbosity-dependent scope or a second entry point. But this is the commit that removes the reason it's there, and switch.rs / alias.rs already narrow to Referenced on the same path for the same reason. Worth narrowing here or naming as a follow-up in the PR body, rather than leaving the doc comment carrying a justification that now only holds at -v.

"Everything else" doesn't hold for a multi-key pre-* table — inline comment on the src/cli/mod.rs line.

Smaller note

tests/integration_tests/step_alias.rs, the doc comment on test_alias_inherits_stdin, still ends "Only hooks have a documented JSON-on-stdin contract; aliases must leave stdin alone." Both halves are now false — there is no such contract, and the hooks-vs-aliases distinction the test was written to defend no longer exists. The test itself is still correct and arguably stronger now: its !combined.contains("\"branch\"") assertion is a second pin on the removal. Outside the diff, so no suggestion; happy to push it.

What I verified

On cd5e135 (the merged head):

• Nothing the removal orphaned is left behind: posix_command_separator still has its spawn_detached_unix caller, Write in output/concurrent.rs is still used by the two writeln! sites, and shell_escape in process.rs still has callers — so the clean clippy run is clean for the right reason rather than by suppression.
• The three hook.md mirrors carry the new ## Interactive hooks text, and nothing in the tree links the removed #json-context anchor. check-docs is green on this head.

On 897fcc7 (the change itself, unaltered by the merge):

• context_json has exactly two remaining references in src/, both in commands/for_each.rs. TemplateContext::to_json's only non-test caller is the same file — the to_json hit in testing/mock_commands.rs is a different type.
• execute_shell_command has one caller (run_one_command) and spawn_detached has two (spawn_background_removal, sweep_stale_trash); all updated. wait_for_valid_json has no remaining users, so deleting it is right.
• The shell_exec.rs Isolated / Shared-tty bullets are accurate: forward_signals() has exactly two call sites, for_each.rs (Isolated, paired with stdin_bytes) and handlers.rs (Shared-tty, paired with inherit_stdin), so "the only Cmd call site with this pair" holds literally.
• spawn_detached's behaviour is byte-identical for both callers: with context_json: None, build_printf_pipe_command returned command unchanged and the Windows PowerShell branch fell through to shell.command(command) — exactly what both paths do now. So dropping that escaping machinery can't reach the removal path.

test (linux|macos|windows), lint, code-coverage and the advisory affected tests jobs are all in flight on cd5e135 as I write this; I haven't seen them land.

Data-safety surface

Flagging per this repo's review guidance (.claude/skills/running-tend/references/review-pr.md § Data-Loss Surface: Hold for Human Review, which holds on a diff that edits a file containing one of the listed destructive operations): the diff edits src/commands/process.rs, which builds the rm -rf trash-sweep command and the staged wt remove command, and src/output/handlers.rs's spawn_background_removal. Nothing widens — both spawn_detached callers already passed None, so the spawned removal command is unchanged — but it's a maintainer call rather than a bot one. Moot for approval here in any case, since this is self-authored.

[worktrunk-bot]

The three new commits close the last open finding and the deferred note; nothing new on this increment, and no concerns remain from my side. Self-authored, so this stands in for an approval rather than being one.

• The Everything else set-complement (src/cli/mod.rs and its three mirrors) — the suggested carve-out landed verbatim in all four, and the sentence now matches the code: map_to_step returns Concurrent for a table with two or more entries whatever the hook type, and run_concurrent_group spawns each child with Stdio::null() (output/concurrent.rs, the "Each child gets a closed stdin" comment). No stale copy of the old sentence is left anywhere in the tree, and no --help snapshot carries this text, so the doc-sync regen was the whole obligation. Thread resolved.
• VarScope::All — the new prepare_steps comment states the cost rather than paying it, which is the right disposition for this PR. I re-checked the claim it rests on: both format_hook_variables call sites are behind verbosity() >= 1 (command_executor.rs's step announcer and hooks.rs's print_background_variable_table), so "buys nothing at default verbosity" is literally true.
• posix_command_separator — the #[cfg(unix)] is correctly scoped: the only caller is the format!("{{ {}{} }} &", …) line inside spawn_detached_unix, itself #[cfg(unix)], and the unit test is gated to match.

The CHANGELOG.md clause has the same general-claim-then-exception-list shape the docs sentence just lost, but its parenthetical names concurrent-group children explicitly, so I still read it as fine and am not reopening it.

test (linux|macos|windows), code-coverage, and the advisory affected tests jobs are in flight on 77528d9 as I write; lint, check-docs, fast-checks, and feature-check are green. test (windows) is the one that would catch the cfg gating if I've read it wrong.

[worktrunk-bot]

Both notes from the last review are on the branch as 77528d9, and CI has now landed green on that head — test (linux|macos|windows), lint, check-docs, code-coverage, codecov/patch and codecov/project all pass. (The review above was written while those were still in flight.)

Two things it doesn't cover:

• The follow-up is in the PR body, not just the code comment — a Follow-up, deliberately not in this PR section naming the VarScope::All cost, why narrowing it wants its own PR (a verbosity-dependent scope or a second entry point for the listing and preview paths, either a behavior change in a hot path unrelated to stdin), and default_branch()'s possible git ls-remote as the sharp end of it.
• test_alias_inherits_stdin's doc comment is fixed too, since you offered. Both false halves are gone; it now says the "branch" assertion pins the rule for every foreground step rather than a hooks-vs-aliases distinction. The assertion message went with it — it still read "the hook JSON context".

@max-sixty this should clear the change request from your review: JSON-on-stdin is gone outright, which was the ask.