perf(switch): compute only the template vars --execute names, and give the context a type - #3628
Conversation
The context map at the `--execute` call site feeds `expand_template` and
nothing else. The payload reaches the child as a shell string through the
EXEC directive file (or `sh -c`), never as JSON on stdin, and `--execute`
renders no `-v` variables table. It still passed `referenced: None`, so
every standard var was computed and thrown away.
`var_default_branch` is the one with teeth. On a clone with no
`refs/remotes/origin/HEAD` and no cached `worktrunk.default-branch` it
falls through to `git ls-remote`, so `wt switch feature -x 'echo hi'`
reached the network for a variable the command never names. Traced on a
scratch repo in that state: 13 subprocesses including one `ls-remote`,
now 8 with none. `-x 'echo {{ commit }}'` still resolves `commit`, and
only `commit`.
`referenced_vars_for_templates` unions minijinja's `undeclared_variables`
across the command and its trailing args, two expansion positions over
one map. It doesn't error on an unparsable template, because
`validate_switch_templates` already parsed both positions before the
switch ran and reports a syntax error against the position that carried
it.
`build_hook_context`'s doc comment said "hooks pass None", describing the
callers rather than the rule, which is how `--execute` inherited `None`
unexamined. It now states the discriminator: `Some` when the map only
feeds `expand_template`, `None` when a child reads it as JSON or the
command prints the variable listing. The two template-preview paths
(`render_hook_commands`, `wt config alias`) keep `None`, where filtering
would be correct but saves nothing an interactive display would notice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`build_hook_context` returned a bare `HashMap<String, String>`, so every caller had to know four separate things about it: how to reborrow it as `HashMap<&str, &str>` for `expand_template` (five copies), how to serialize it for a child's stdin (four copies, two carrying the same `expect` string), what to insert into it afterwards, and whether it had been filtered. `TemplateContext` owns all of that. `expand`, `to_json`, `insert`, `get` and `remove` are the interface; the map itself is private. It serializes transparently, so the JSON a hook child reads on stdin and the pipeline spec the background runner deserializes are the same flat object as before. The filter argument becomes `VarScope` rather than `Option<&BTreeSet<String>>`. `None` was the value you got by copying the line above, which is how `--execute` came to compute every variable and discard most of them; `VarScope::All` has to be typed out, and its doc says when it's the right answer. The same enum replaces the second `Option<&BTreeSet<String>>` on `format_alias_variables`, which meant the same thing: which vars render as dim `(unused)`. `llm::TemplateContext` (private, the data an LLM prompt template renders against) becomes `PromptContext`, so the name means one thing. No behavior change: the gate passes 4606 tests with no snapshot movement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewed the refactor and it looks sound. TemplateContext and VarScope cleanly consolidate what was scattered across the callers — the HashMap<&str, &str> reborrow, the serde_json boundary, and the (unused)/(unset) rendering now each have one home — and every VarScope::All site is behavior-preserving against the old None. The --execute narrowing is the part with teeth, and it holds: at that call site the context feeds only expand() (never a child's stdin JSON, and --execute renders no -v table), so scoping to the vars the command and trailing args actually name can't drop a value anything downstream would read — and default_branch still resolves (and may ls-remote) when the command names it. Built it locally: test_switch_execute_computes_only_referenced_vars, the two new referenced_vars_for_templates unit tests, and the rest of the expansion suite pass, no snapshot movement.
Not approving via empty body only because there are two red checks in the rollup — but they aren't test failures. affected tests (linux/windows, advisory) aborted inside cargo affected run while parsing the diff:
Error: git diff stdout was not valid UTF-8: invalid utf-8 sequence of 1 bytes from index 136455
No test executed (no report.json was produced). That's a deterministic cargo-affected limitation on a non-UTF-8 byte somewhere in the collect_sha..HEAD range (the log also notes it's 8 commits behind collect), not anything in this change — the same advisory checks pass green on #3625 and #3600. The required test (…) matrix and lint are still in progress.
wt switch --executebuilt its template context withreferenced: None, so every invocation resolved every standard variable whether or not the command named one. That context feedsexpand_templateand nothing else: the payload reaches the child as a shell string through the EXEC directive file (orsh -c), never as JSON on stdin, and--executerenders no-vvariables table. Every unnamed variable was a git subprocess whose result was discarded.var_default_branchis the one with teeth. On a clone with norefs/remotes/origin/HEADand no cachedworktrunk.default-branch, it falls through togit ls-remote, sowt switch feature -x 'echo hi'reached the network for a variable the command never mentions. Traced on a scratch repo in that state:--executeecho hils-remoteecho {{ commit }}echo {{ default_branch }}referenced_vars_for_templatesunions minijinja'sundeclared_variablesacross the command and its trailing args, which are two expansion positions over one context map. It does not error on an unparsable template:validate_switch_templateshas already parsed both positions againstValidationScope::SwitchExecutebefore the switch runs, and reports a syntax error against the position that carried it.The second commit: why
Nonewas reachable at allbuild_hook_contextreturned a bareHashMap<String, String>, so each of its seven callers had to know four separate things about the result: how to reborrow it asHashMap<&str, &str>forexpand_template(five copies), how to serialize it for a child's stdin (four copies, two carrying the sameexpectstring), what to insert into it afterwards, and whether it had been filtered. The doc comment on that last point said "hooks passNone", which described the callers rather than the rule, so--executeinheritedNoneby sitting next to code that needed it.TemplateContext(src/config/expansion.rs) owns all of it, withexpand/to_json/insert/get/removeas the interface and the map private. It serializes transparently, so the JSON a hook child reads on stdin and thePipelineSpecthe background runner deserializes are the same flat object as before.The filter argument becomes
VarScoperather thanOption<&BTreeSet<String>>.VarScope::Allhas to be typed out, and its doc states the discriminator:Referencedwhen only the templates read the context,Allwhen a child reads the JSON or the command's output is the variable listing. The same enum replaces the secondOption<&BTreeSet<String>>onformat_alias_variables, which meant the same thing (which vars render as dim(unused)).Two changes fell out of that rather than being planned.
src/llm.rshad its own privateTemplateContextfor LLM prompt data, which would have made two types of that name in one crate; it is nowPromptContext. Andexpand_shell_templateshrank to a one-line wrapper but stays, because it still fixes POSIX for its four callers and converts the error type.Not included:
render_hook_commands's preview path reimplements whatprepare_stepsdoes (build context, inserthook_type, inserthook_name, defaultargs, expand POSIX), so the preview drifts from what actually runs whenever the execution path gains a context key. That toucheswt hookoutput and wants its own change.Testing
test_switch_execute_computes_only_referenced_varsreadsvar_*spans out of the-vvtrace.jsonland pairs the variable-free case with a{{ commit }}control, so an empty span set cannot pass by the trace simply missing them. Reverted toNoneit fails withgot ["var_commit", "var_default_branch", "var_primary_worktree", "var_remote"]. Two unit tests coverreferenced_vars_for_templatesdirectly (union across positions, unparsable template contributes nothing).The refactor is covered by the existing suite rather than new tests, which is the point: 4606 tests pass with no snapshot movement, so no user-visible output changed.