test(hook-plan): isolate approval test from the real user config dir - #2853
Merged
Conversation
`approve_readonly_drops_unapproved_project_keeps_user` called `approve_command(.., None)`, which resolves the approvals path to the real user config dir and writes there. Outside a sandbox `$HOME` is writable so it silently passed (while polluting the user's `approvals.toml`); the nix build sandbox makes that dir unwritable, so the test panicked with `Permission denied`. This had been latent on `main` since #2806 — it only started tripping the gate now that #2849 made a PR touch `Cargo.{toml,lock}`, the trigger for the `nix-flake` job. Point the call at a tempdir-backed approvals path via the `Some(&path)` parameter `approve_command` already accepts, matching the in-file pattern in `src/config/approvals.rs` tests. The two sibling tests only read via `Approvals::load()` (returns default when no file exists), so they pass in the sandbox unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 21, 2026
max-sixty
added a commit
that referenced
this pull request
May 21, 2026
The `Approvals` and `UserConfig` mutation methods took an `Option<&Path>`: `None` meant "resolve the global config path", `Some(p)` meant "use `p`". `None` was a footgun. A unit test that passed `None` silently wrote to the developer's real `~/.config/worktrunk/`: it passed wherever `$HOME` was writable and failed only in a sandbox with no writable config dir. That is the class of bug behind the recent `nix-flake` failure (#2853). This drops the `Option`. All ten mutation methods (`Approvals::{approve_command, approve_commands, revoke_project, clear_all, with_locked_mutation}` and `UserConfig::{set_skip_shell_integration_prompt, set_skip_commit_generation_prompt, set_project_worktree_path, set_commit_generation_command, with_locked_mutation}`) now take `&Path`. Production callers resolve the path explicitly through two new helpers: `require_approvals_path()` and `require_config_path()`, the `Result`-returning counterparts of `approvals_path()` / `config_path()`, matching the `require_*` accessor convention. A test can no longer reach the real config dir without typing the path; the resolver is no longer a silent default. `HookPlan::approve` also skips `Approvals::load()` on the `--yes` path. That path approves every project command unconditionally and persists nothing, so the load was dead work. Skipping it removes a useless I/O and keeps `wt merge --yes` working when `approvals.toml` is malformed. `tests/CLAUDE.md` gains a "Config Isolation for In-Process Unit Tests" section; the `approvals_path()` doc is updated to describe the lib-crate-only `#[cfg(test)]` guard. ## Testing Full pre-merge gate green (3811 tests, fmt, clippy, doctests). The three `hook_plan` tests pass under a non-writable-`HOME` sandbox repro of the build-sandbox condition. One coverage note: `require_approvals_path()` / `require_config_path()` each have an error branch (`ok_or_else(|| ConfigError("Cannot determine …"))`) that fires only when no config directory is resolvable. CI always sets `$HOME` / `WORKTRUNK_*_PATH`, so that branch is unreachable in tests, and `codecov/patch` may flag those two lines. The same closure existed before this change, inside `with_locked_mutation`; the refactor moves it into the new helpers, where it counts as patched lines. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
max-sixty
added a commit
that referenced
this pull request
May 23, 2026
…2853) The `nix-flake` CI job was red on `main`: the unit test `commands::hook_plan::tests::approve_readonly_drops_unapproved_project_keeps_user` panicked with `Failed to create config directory: Permission denied`. The test called `approve_command(.., None)`. The `None` for `approvals_path` makes `Approvals` resolve the path through `approvals_path()`, which — in the lib crate compiled in non-test mode — points at the real `~/.config/worktrunk/` directory. `approve_command` then `create_dir_all`s it. Outside a sandbox `$HOME` is writable, so the test silently passed while writing a stray `[projects.proj]` entry into the user's `approvals.toml`. The nix build sandbox has no writable config dir, so it panicked instead. This was latent on `main` since #2806 (which added the test). It only started tripping the gate now because the `nix-flake` job runs when a PR touches `Cargo.{toml,lock}`, and #2849 was the first to do so — so `nix-flake` failed there for a reason unrelated to that PR's change. The fix points `approve_command` at a `tempfile::tempdir()`-backed approvals path via the `Some(&path)` parameter it already accepts — no process-env mutation (`tests/CLAUDE.md` bans it), matching the in-file pattern in `src/config/approvals.rs`'s own tests. Same class of fix as #2615 and #2624. The two sibling tests in `hook_plan.rs` only *read* via `Approvals::load()`, which returns the default when no file exists — never creating a directory or writing — so they pass in the sandbox unchanged and were left as-is. Verified by reproducing the sandbox condition locally (test binary run with `HOME` / `XDG_CONFIG_HOME` pointed at a non-writable dir): the target test fails before the fix and passes after, and all three `hook_plan` tests still pass under a plain `cargo test`. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
max-sixty
added a commit
that referenced
this pull request
May 23, 2026
The `Approvals` and `UserConfig` mutation methods took an `Option<&Path>`: `None` meant "resolve the global config path", `Some(p)` meant "use `p`". `None` was a footgun. A unit test that passed `None` silently wrote to the developer's real `~/.config/worktrunk/`: it passed wherever `$HOME` was writable and failed only in a sandbox with no writable config dir. That is the class of bug behind the recent `nix-flake` failure (#2853). This drops the `Option`. All ten mutation methods (`Approvals::{approve_command, approve_commands, revoke_project, clear_all, with_locked_mutation}` and `UserConfig::{set_skip_shell_integration_prompt, set_skip_commit_generation_prompt, set_project_worktree_path, set_commit_generation_command, with_locked_mutation}`) now take `&Path`. Production callers resolve the path explicitly through two new helpers: `require_approvals_path()` and `require_config_path()`, the `Result`-returning counterparts of `approvals_path()` / `config_path()`, matching the `require_*` accessor convention. A test can no longer reach the real config dir without typing the path; the resolver is no longer a silent default. `HookPlan::approve` also skips `Approvals::load()` on the `--yes` path. That path approves every project command unconditionally and persists nothing, so the load was dead work. Skipping it removes a useless I/O and keeps `wt merge --yes` working when `approvals.toml` is malformed. `tests/CLAUDE.md` gains a "Config Isolation for In-Process Unit Tests" section; the `approvals_path()` doc is updated to describe the lib-crate-only `#[cfg(test)]` guard. ## Testing Full pre-merge gate green (3811 tests, fmt, clippy, doctests). The three `hook_plan` tests pass under a non-writable-`HOME` sandbox repro of the build-sandbox condition. One coverage note: `require_approvals_path()` / `require_config_path()` each have an error branch (`ok_or_else(|| ConfigError("Cannot determine …"))`) that fires only when no config directory is resolvable. CI always sets `$HOME` / `WORKTRUNK_*_PATH`, so that branch is unreachable in tests, and `codecov/patch` may flag those two lines. The same closure existed before this change, inside `with_locked_mutation`; the refactor moves it into the new helpers, where it counts as patched lines. --------- Co-authored-by: Claude Opus 4.7 (1M context) <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.
The
nix-flakeCI job was red onmain: the unit testcommands::hook_plan::tests::approve_readonly_drops_unapproved_project_keeps_userpanicked withFailed to create config directory: Permission denied.The test called
approve_command(.., None). TheNoneforapprovals_pathmakesApprovalsresolve the path throughapprovals_path(), which — in the lib crate compiled in non-test mode — points at the real~/.config/worktrunk/directory.approve_commandthencreate_dir_alls it. Outside a sandbox$HOMEis writable, so the test silently passed while writing a stray[projects.proj]entry into the user'sapprovals.toml. The nix build sandbox has no writable config dir, so it panicked instead.This was latent on
mainsince #2806 (which added the test). It only started tripping the gate now because thenix-flakejob runs when a PR touchesCargo.{toml,lock}, and #2849 was the first to do so — sonix-flakefailed there for a reason unrelated to that PR's change.The fix points
approve_commandat atempfile::tempdir()-backed approvals path via theSome(&path)parameter it already accepts — no process-env mutation (tests/CLAUDE.mdbans it), matching the in-file pattern insrc/config/approvals.rs's own tests. Same class of fix as #2615 and #2624.The two sibling tests in
hook_plan.rsonly read viaApprovals::load(), which returns the default when no file exists — never creating a directory or writing — so they pass in the sandbox unchanged and were left as-is.Verified by reproducing the sandbox condition locally (test binary run with
HOME/XDG_CONFIG_HOMEpointed at a non-writable dir): the target test fails before the fix and passes after, and all threehook_plantests still pass under a plaincargo test.