refactor(hooks): derive the hook preview from the execution path - #3635
Merged
Conversation
`wt hook show --expanded` rebuilt the pipeline template context by hand, duplicating what `prepare_steps` already does. It now prepares its commands through `prepare_steps` itself, so a context key the execution path gains reaches the preview with no second edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adding `source` to the listing renderer left `approval_context` encoding the same user-vs-project discriminator, so both sites now call one `needs_approval`. Also pins the relocated syntax gate with a test that reaches it — `wt hook <type>` cannot, because the CLI pre-parses every template for shorthand routing and errors first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
max-sixty
marked this pull request as ready for review
July 28, 2026 22:16
worktrunk-bot
approved these changes
Jul 28, 2026
max-sixty
added a commit
that referenced
this pull request
Jul 28, 2026
Follow-up to #3635, from two reviews that landed after it merged. ## The `vars.*` preview was worse than #3635 claimed `render_template_preview` short-circuits on `template_references_var(template, "vars")`, returning the raw template. So one `vars.` token disabled expansion for the entire command: ``` pre-commit = "deploy --branch={{ branch }} --repo={{ repo }} --env={{ vars.env }}" before: deploy --branch={{ branch }} --repo={{ repo }} --env={{ vars.env }} after: deploy --branch=main --repo=repo --env={{ vars.env }} ``` #3635 described this as "a `vars.*` template renders raw", which is true but understates it: `{{ branch }}` and `{{ repo }}` stopped expanding too, in a command whose whole job is to show the expansion. That short-circuit predates #3635 and has been degrading `wt hook <type> --dry-run` the same way; #3635 only extended it to `wt hook show --expanded`. The fix is at the source rather than at either caller. A preview now injects a stand-in object for `vars` that renders each reference back as itself, nested access included (`{{ vars.config.port }}` round-trips), while every other variable expands normally. `VarsMode::Resolve` keeps execution reading real values from git config; only previews pass `VarsMode::Literal`. A preview also no longer spawns the git read that resolving `vars` required. `vars.*` stays literal on purpose: those values are read when the step runs, after an earlier step in the pipeline may have written them, so a value resolved at preview time can differ from the one the run uses. Nothing covered this, which is why the suite stayed green through the regression. `test_hook_show_expanded_matches_dry_run` now sets a var and asserts the listing and the dry-run both leave it alone while expanding `{{ branch }}` beside it. ## The syntax gate is a type error now #3635 moved the template syntax check out of `prepare_steps` into a free `validate_pipeline_syntax` that both execution funnels had to remember to call. `prepare_steps` now returns a `PreparedPipeline` the caller must resolve: `.validated()` for the paths that run hooks, `.into_unvalidated()` for the listing, which annotates a broken template in place rather than blanking itself. Forgetting is a compile error, the same property `ApprovedHookPlan` gives hook approval. ## Smaller items Four cross-references went stale when the syntax check moved: `PreparedCommand.template`, `validate_template_syntax`, the `switch.rs` skip comment, and `HOOK_INFRASTRUCTURE_VARS` (which still named two deleted functions). The `--expanded` behavior is now documented in the sentence that already owns `{{ vars.<key> }}` semantics, with its three generated mirrors regenerated. `PreparedStep::commands()` replaces two hand-rolled matches in `hooks.rs`. `default_branch` moves inside `build_manual_hook_template_vars` — only the commit-hook arm reads it, and resolving it can cost a `git ls-remote` on a fresh clone, so the other eight hook types no longer pay for it. The listing carries its expansion state in an `Option<String>` instead of re-deriving "was this expanded?" from whether a context exists. > _This was written by Claude Code on behalf of max_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 task
max-sixty
pushed a commit
that referenced
this pull request
Aug 7, 2026
…#3765) Nightly sweep finding: two `.config/clawpatch/features/*.json` entrypoints name symbols that no longer exist. **`feat_custom_template_expansion.json`.** `expand_command_template` was deleted by #3635 ("derive the hook preview from the execution path"), which folded the preview render into `render_template_preview` in `src/commands/command_executor.rs` and left `expand_template` in `src/config/expansion.rs` as the single runtime render. The feature file kept pointing at the old name, so the one thing it exists to do — send a reader to the code that renders a project-supplied template before the approval gate sees it — lands nowhere. - **Entrypoint** → `src/config/expansion.rs::expand_template`. The module doc calls it "a single generic function" for template rendering, and it's what the feature's own summary is about ("expansion happens before the approval gate sees the final string"). - **`ownedFiles` reordered and re-reasoned.** `expansion.rs` moves first and its reason now names the render functions. `command_executor.rs` is added — that's where #3635 put `render_template_preview` and the foreground step pipeline, so the file the code moved *into* was missing from a file list that still described where it came from. `hook_commands.rs` stays, with its reason updated to what it actually holds now (`run_hook`, the `hook show --expanded` rows). **`feat_custom_switch_resolve.json`.** Entrypoint `handle_switch` → `handle_switch_command`. The function was renamed when #3049 moved switch/remove orchestration out of `main.rs`; the definition today is `pub fn handle_switch_command` at `src/commands/worktree/switch.rs`. This second one is why the sweep's original audit reported only one stale symbol: it grepped each `entrypoints[].symbol` as a plain substring, and `handle_switch` matches inside `handle_switch_command` (and `handle_switch_output`, `handle_switch_created_output`, …), so a renamed symbol whose new name merely extends the old one reads as present. Re-run anchored on a definition — `\b(fn|struct|enum|trait|type|const|static|mod)\s+<symbol>\b` inside the declared `path` — all 14 features now resolve, and no other entrypoint has this shape. Following the convention of the last edit to this directory (3554f49, which fixed `interrupt_exit_code` → `interrupt_signal` in the signal-handling feature), symbol names are corrected in place and `updatedAt` is left alone. No test accompanies this — nothing in the suite reads `.config/clawpatch/`, which is why both references went stale silently. A sync test is conceivable, but it would be a new check on a hand-authored threat-model index whose schema this repo doesn't own; I'd rather flag the idea than build it unasked. If one is ever added, it should anchor on the definition, not a substring grep, for the reason above. --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wt hook show --expandedrebuilt the hook pipeline's template context by hand inexpand_command_template, duplicating whatprepare_stepsalready does for what actually runs: the samebuild_hook_contextcall, the samehook_typeandhook_nameinserts, the sameargsdefault, the same POSIX escape mode. Two copies of one rule, so the preview drifted from the executed command whenever the execution path gained a context key, with nothing to catch it.The listing now prepares its commands through
prepare_stepsitself (hook_command_rowsinhook_commands.rs) and renders them throughrender_template_preview, the rendererwt hook <type> --dry-runalready used.prepare_stepsis the sole producer of hook command contexts, so a key added there reaches both with no second edit.The
argsdivergenceThe old preview inserted
args = "[]"unconditionally;prepare_stepsdefaults it only when unset, because manualwt hook <type>supplies real args upstream viaextra_vars. The shared path keeps the conditional default and the listing simply does not supplyargs. The values coincide (a listing has no CLI args to forward, which is exactly what the default encodes), and keeping the conditional form means the rule stays written once, in the place that has a caller who needs the other branch.Where the syntax check went
prepare_stepsused to reject an unparsable template, so a pipeline that could not render in full never started. A listing wants the opposite:wt hook showis what you run when your hooks are broken, so it annotates the bad template in place and shows the rest.That check is an execution policy rather than part of building a command, so it moved out of
prepare_stepsintovalidate_pipeline_syntax, called by the two funnels every hook-running path goes through:prepare_and_check(foreground, background, dry-run, filtered) andrender_planned(the plan-backed hooks behindexecute_planned_hookandregister_planned). Both are mutation-verified: removing either call fails a test.A newtype that made forgetting the gate a compile error would be stronger, but it threads a wrapper through
SourcedStep,ForegroundStep, and the background pipeline spec for a guard whose failure mode is degraded fail-fast rather than incorrectness (a syntax error still surfaces when its step renders).User-visible changes
Preview expansion errors now name the hook (
Failed to expand project:lint: ...) instead of the generichook preview.A template referencing
vars.*renders raw in--expanded, matching--dry-run, where before it resolved against git config at preview time. Raw is the honest preview: those values resolve when the step runs, and an earlier step in the pipeline may write them.Tests
test_hook_show_expanded_matches_dry_runpins the listing and the dry-run to the same rendering ofhook_type,hook_name, andargs.test_foreground_pipeline_syntax_error_aborts_before_first_steppins the relocated gate; it runs throughwt merge's pre-commit hooks becausewt hook <type>cannot reach it (the CLI pre-parses every template for shorthand-argument routing and errors first).Also folded in: adding
source: HookSourceto the listing renderer leftapproval_context: Option<(&Approvals, Option<&str>)>encoding the same user-vs-project discriminator, so both sites now call oneneeds_approval.