Skip to content

fix(hooks): expand everything but vars.* in a preview - #3638

Merged
max-sixty merged 4 commits into
mainfrom
hook-preview-shared-context
Jul 28, 2026
Merged

fix(hooks): expand everything but vars.* in a preview#3638
max-sixty merged 4 commits into
mainfrom
hook-preview-shared-context

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

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

max-sixty and others added 3 commits July 28, 2026 15:49
render_template_preview short-circuited the whole template when it
mentioned vars anywhere, so one {{ vars.x }} reference made
`wt hook show --expanded` and `--dry-run` a no-op for the rest of the
command. A preview now injects a stand-in that renders each reference
back as itself, nested access included, while every other variable
expands.

Also fixes stale cross-references the syntax-gate move left behind and
adopts PreparedStep::commands at two hand-rolled sites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
prepare_steps returns a PreparedPipeline the caller must resolve:
.validated() for the paths that run hooks, .into_unvalidated() for the
listing that annotates a broken template in place. A new execution path
can no longer skip the check by omitting a line.

Also folds default_branch into build_manual_hook_template_vars (only the
commit-hook arm reads it, and resolving it can cost a git ls-remote), and
carries the listing's expansion state in an Option rather than
re-deriving it from whether a context exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two stale references to validate_pipeline_syntax slipped in — the same class of stale cross-reference this PR set out to fix, except here they point at the function the PR removes. Both should name PreparedPipeline::validated, the path that now runs the check. Suggestions inline. Otherwise the change reads clean: the LiteralVars stand-in round-trips nested access, the VarsMode split keeps execution reading real values, and the #[must_use] PreparedPipeline gate is a nice unforgeable-by-construction property.

Comment thread src/commands/worktree/switch.rs Outdated
Comment thread src/config/expansion.rs Outdated
Three comments still pointed at validate_pipeline_syntax, which this
branch replaces — the same stale-cross-reference class the branch fixes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@max-sixty
max-sixty merged commit 9e7ad01 into main Jul 28, 2026
47 checks passed
@max-sixty
max-sixty deleted the hook-preview-shared-context branch July 28, 2026 23:44
max-sixty added a commit that referenced this pull request Jul 29, 2026
#3638 changed the preview renderer so a template expands everything
around a `{{ vars.<key> }}` reference and leaves only that reference
literal. `wt config alias dry-run` shares that renderer, but its
`--help` text and the `handle_alias_dry_run` docstring still described
the previous all-or-nothing behavior, where a single `vars.` token left
the whole template raw.

This corrects both. The docstring's mention of a separate
syntax-validation step goes with them: expansion reports syntax errors
itself, so there is no longer a distinct validation pass to describe.

No behavior change. The `dry-run` long help isn't rendered into any
generated doc or snapshot, so no mirrors move with it.

> _This was written by Claude Code on behalf of max_

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@worktrunk-bot worktrunk-bot mentioned this pull request Jul 29, 2026
max-sixty added a commit that referenced this pull request Jul 29, 2026
)

`wt hook show` decided whether a section had printed anything from
whether the config held an entry for each hook type, not from whether
any command was rendered. A hook type declared with an empty command
list has an entry but no commands, so the section printed its heading
and then stopped:

```console
$ cat .config/wt.toml
post-switch = []

$ wt hook show
PROJECT HOOKS @ /path/.config/wt.toml
```

Both sections carried it, since the loop and the `(none configured)`
fallback were duplicated in `render_user_hooks` and
`render_project_hooks`. `render_hook_commands` now reports whether it
wrote any rows, and the loop plus fallback live once in
`render_hook_section`, which both callers delegate to. The flag means
"something was printed", which is what the `(none configured)` line
claims.

The execution path was already correct: an empty list announces nothing,
and the JSON output omits it. Verified against the built binary across
four cases (project-only empty, user-only empty, an empty list alongside
a real hook, and a filter naming the empty type), and pinned by
`test_hook_show_empty_command_lists`, which covers both the user and
project halves. Reverting the flag to its old meaning fails the test on
both.

Also corrects a docstring in the same file that still described the
preview's pre-#3638 behavior ("shows a `vars.*` template raw"), the same
drift #3639 fixed for `wt config alias dry-run`.

> _This was written by Claude Code on behalf of max_

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants