fix: harden hook filtering, approvals store, and template expansion#2841
Merged
Conversation
A mixed-source invocation like `wt hook pre-merge project:deploy user:lint` collapsed all parsed filters into a flat name set and applied it uniformly to every source. With overlapping command names in both configs, project `lint` and user `deploy` were also retained — and under `--yes` an unrequested project hook could execute simply because it shared a name with a user-scoped filter, bypassing the per-source approval intent. `ParsedFilter` gains `matches_command`, combining source-matching with name-matching. `filter_step_by_name` and `approve_hooks_filtered` now filter source-aware: a command is kept only when a filter both matches its source and (has an empty name or equals the command name). Adds a mixed source/name regression test asserting only the two explicitly selected hooks run. Co-Authored-By: Claude <noreply@anthropic.com>
`handle_command_error` checks `interrupt_exit_code()` before the
`FailureStrategy` match, so a signal-derived child exit aborts the loop
even under `FailureStrategy::Warn`. The existing Warn unit test only
used `signal: None`, and the signal integration tests cover `for_each`
and a pre-merge FailFast path — none exercise the Warn arm. A refactor
moving the interrupt check below the match would silently reintroduce
the swallowing bug while passing the current suite.
Adds `test_handle_command_error_warn_signal_aborts`: a SIGTERM-derived
`ChildProcessExited` with `FailureStrategy::Warn` must return
`AlreadyDisplayed { exit_code: 143 }`, not `Ok(())`.
Co-Authored-By: Claude <noreply@anthropic.com>
`validate_template` caught undefined variables only through a trial
render. Under MiniJinja's `SemiStrict` undefined behavior, an undefined
value in a truthiness test (`{% if targte %}`) is permitted and renders
as false, so a typo in a project-supplied command's conditional passed
validation and silently dropped or selected the wrong command fragment.
Before the render pass, parse the template and diff
`undeclared_variables(false)` against the scope's available variables
(plus `vars`). Any leftover name — including one used only as an `if`
predicate — is reported as an undefined-value error. The render pass
still runs afterward for syntax, filter, and render-time errors.
`{% set %}` / `{% for %}` targets are excluded by `undeclared_variables`,
so loop and assignment templates still validate.
Adds a regression test for `{% if targte %}` in pre-merge scope.
Co-Authored-By: Claude <noreply@anthropic.com>
`Approvals::save_to` used `std::fs::write`, which truncates the target before writing. A disk-full error, crash, or reader racing the rewrite could leave an empty or partial `approvals.toml`; an empty file deserializes to the default (empty) trust store, silently dropping every approval. Write to a temp file in the same directory, flush and `sync_all`, then `persist` (atomic rename) over `approvals.toml`. A failed write now leaves the original file untouched — favoring the failing variant over silent data loss, per the Data Safety policy. Adds a Unix test asserting a save into a read-only directory errors and leaves the prior approvals intact. Co-Authored-By: Claude <noreply@anthropic.com>
`Approvals` and `ApprovedProject` accepted unknown TOML fields. A schema-valid but semantically wrong file — `[project."..."]` instead of `[projects."..."]`, or `approved-command` instead of `approved-commands` — parsed successfully into an empty or partial trust store, silently dropping approvals. Add `#[serde(deny_unknown_fields)]` to both structs so a misspelled key aborts the load instead of degrading to "nothing approved". The fix applies only to `approvals.toml`; the `config.toml` fallback deserializes `UserConfig`, which is unaffected. Adds tests for an unknown top-level table and an unknown per-project key. Co-Authored-By: Claude <noreply@anthropic.com>
`copy_approved_commands_to_approvals_file` returned a benign `Ok(None)` for any existing `approvals.toml` without checking it was parseable. When `wt config update` then removed legacy `approved-commands` from config.toml, a corrupt `approvals.toml` left the user with neither a usable trust store nor the legacy fallback. The `exists()` check was also outside the approvals lock, so a concurrent approval could create the file between the check and the migration write and be overwritten. Acquire the approvals config lock before the existence check, and validate that an existing `approvals.toml` parses before returning `Ok(None)` — an unparseable file now errors so `config update` aborts before rewriting config.toml. Adds a test for an invalid existing `approvals.toml`. Co-Authored-By: Claude <noreply@anthropic.com>
`expand_template` loaded git `vars` state only when the raw template
contained the substring `vars.`. Supported MiniJinja access forms —
`{{ vars["env"] }}` and bare `{{ vars }}` — have no `vars.` substring,
so the state was never injected and rendering failed as undefined or
fell through to defaults.
Detect the top-level `vars` reference with
`tmpl.undeclared_variables(false)` after parsing the template, matching
the AST-based approach the Code Quality guidance mandates over string
detection. This supports every access form and drops the
false-positive concern that motivated the substring check.
Adds expansion tests for `{{ vars["env"] }}` bracket access and a
`{% if vars %}` conditional.
Co-Authored-By: Claude <noreply@anthropic.com>
`test_validate_template_scope_rejects_out_of_scope_vars` uses the misspelling `targte` as an out-of-scope variable fixture. The `crate-ci/typos` pre-commit hook autocorrected it to `target` on every run, turning the fixture into a valid pre-merge variable and breaking the test (`validate_template` returned `Ok`, the `unwrap_err()` panicked). Whitelist `targte` in `.typos.toml` alongside the existing intentional test typos (`deplyo`, `comit`, `siwtch`, `brnach`). Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 20, 2026
max-sixty
added a commit
that referenced
this pull request
May 20, 2026
Conflict in src/config/expansion.rs test_expand_template_vars_data: kept #2841's new `vars["env"]` and `{% if vars %}` assertions, updated all three to B1's ShellEscapeMode::Literal signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
max-sixty
added a commit
that referenced
this pull request
May 20, 2026
The merge of main pulled in a new `HookType::PostStart` reference (a test added by #2841); this branch renamed that variant to `PostCreate`. Textual auto-merge took main's line verbatim, breaking the build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
max-sixty
added a commit
that referenced
this pull request
May 21, 2026
…2851) Correctness and data-safety fixes for worktrunk's config deprecation/migration layer and hook-log layout — surfaced by an automated `clawpatch` review and re-reviewed against the `reviewing-code` checklist. ## Fixes - **Deprecated config sections are no longer dropped when the parent is an inline table.** With `commit = { stage = "tracked" }` alongside a deprecated `[commit-generation]`, migration removed the old section but silently failed to write `[commit.generation]` — `as_table_mut()` returns nothing for an inline table, so the nested insert was skipped after the source had already been removed. Migration now converts an inline-table parent to a standard table before inserting. Real data loss without this. - **Deprecated template variables are rewritten only inside template tags.** The old `\bword\b` regex rewrote any occurrence of a deprecated name — literal command text (`echo repo_root`), `{% set %}` locals, member access — not just genuine `{{ … }}` references. That could corrupt a command on migration, and could collapse two distinct commands to the same normalized string so an un-approved command matched an approved one. The rewrite is now gated on `minijinja`'s parser (`undeclared_variables`) plus a delimiter-aware scan, so only real deprecated references inside `{{ }}`/`{% %}` are rewritten. This is the fix #2841 deliberately deferred to a sibling branch. - **Repo-wide internal logs are written as top-level files.** Branch-agnostic internal-operation logs were written into a top-level `internal/` directory, violating the layout invariant that top-level directories are per-branch trees — `wt config state` would miscategorize `internal/` as a branch. They now write to `internal-{op}.log` files alongside the other shared logs. - **System config is routed through the deprecation gate.** System config used a silent migrate path; it now goes through `check_and_migrate` like user config, so a deprecation in system config surfaces warnings consistently. ## Scope Pure code — the clawpatch review tooling and its state files are excluded. ## Testing `cargo run -- hook pre-merge --yes` — 3825 tests pass, lints clean. > _This was written by Claude Code on behalf of Maximilian Roos_ --------- Co-authored-by: Claude <noreply@anthropic.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.
Correctness and safety fixes for worktrunk's hook-filtering, command-approval, and template-expansion code, surfaced by an automated
clawpatchreview of the project's threat-model surfaces and then independently re-reviewed against thereviewing-codechecklist — all came back SOLID / Clean-design with zero critical issues.Fixes
wt hook pre-merge project:deploy user:lintpreviously matched filter names across the project/user split, so a name given for one source could select an unintended hook from the other. The approval gate and the executor now share one source-scoped predicate, so the executor cannot run a command the gate did not approve.{{ vars["env"] }}and bare{{ vars }}were missed by the oldcontains("vars.")check.{% if %}predicates are now a clear error instead of being silently ignored..typos.tomlallowlist entry for an intentionaltargtetest fixture.Scope
Pure code — the clawpatch review tooling and its state files are intentionally excluded. One related finding (deprecated-variable rewriting that could mis-match an approved command) is fixed more completely on a sibling branch and is deliberately not duplicated here.
Testing
cargo run -- hook pre-merge --yes— 3790 tests pass, lints clean.