fix(hooks): stop yaml_executor unit tests from leaking into ~/.local/state/daft

[avihut]

Summary

• LogStore::for_repo_in(repo_hash, base) parameterizes the state-dir base. Existing for_repo keeps its production behavior by delegating with daft_state_dir().
• HookContext gains an optional state_dir (set via with_state_dir) that execute_yaml_hook_with_rc consults before falling back to the global state dir.
• make_ctx_with_dir now sets state_dir to the same tempdir it already uses as git_dir, so yaml_executor unit tests no longer write under ~/.local/state/daft/jobs/.

Pre-#448 the leak was visible: cwd-based compute_repo_id() made unit tests pollute the current repo's UUID dir under whatever repo the test runner sat in. #448 fixed the cwd issue but left a quieter leak — every cargo test of yaml_executor still wrote a fresh orphan UUID directory into the user's real state dir. This PR closes that.

Out of scope:

• Cleaning up the existing pre-feat: add daft repo remove and daft repo subcommand category #448 feature/new residue. Use daft hooks jobs prune --older-than 1d to clear stale entries.
• The UX gaps around large daft hooks jobs --all output (no aggregate view, no prune --worktree/--hook filter). Tracked separately.

Fixes #478

Test plan

• mise run test:unit — full suite green
• mise run clippy — zero warnings
• mise run fmt:check — clean
• New regression test test_execute_yaml_hook_honors_state_dir_override passes after the fix
• Same test fails (LogStore writes should have landed under the tempdir state base) when the for_repo_in redirect is reverted, confirming it actually catches the regression

🤖 Generated with Claude Code

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 2m 48s —— View job

---

PR Review: fix(hooks): stop yaml_executor unit tests from leaking into ~/.local/state/daft

Tasks:

• Read changed files
• Review src/coordinator/log_store.rs
• Review src/hooks/environment.rs
• Review src/hooks/template.rs
• Review src/hooks/yaml_executor/mod.rs
• Check regression test coverage
• Post final review

---

Summary

The fix is well-designed and correct. The dependency-injection approach (threading state_dir through HookContext rather than using a global) is the right abstraction. The regression test verifies both halves of the contract — writes land in the tempdir AND nothing leaks into the real state dir.

Observations

for_repo_in visibility (src/coordinator/log_store.rs:148)

for_repo_in is pub, but the doc comment calls it out as "Tests use this to redirect writes to a tempdir." It's only used internally — for_repo (same file) and yaml_executor/mod.rs. Since the rest of LogStore's public API is also pub, this is consistent with the existing pattern; but pub(crate) would give compile-time enforcement of the stated "test-only" intent. Strictly optional given this is a binary crate — nothing external can call it — but worth a thought.

make_ctx() leaks more state than before (yaml_executor/mod.rs:561-567)

make_ctx intentionally leaks the TempDir via std::mem::forget. Before this PR, only the daft-id file was written to the forgotten dir. Now that state_dir points to the same tempdir, the LogStore also writes invocation/job records there. The OS cleans it up at process exit, so the "don't mind leaking the dir for the test process duration" rationale still holds — just a heads-up that the forgotten dirs now contain more state.

Regression test correctness — positive call-out (yaml_executor/mod.rs:655–710)

The test is well-structured:

• Uses execute_yaml_hook() (the public entry point), not the internal _with_rc variant, so it exercises the full call path.
• Derives repo_hash from the tempdir after the fact, so it can locate exactly where the LogStore would have written — no hard-coded paths.
• The leak check uses that same UUID-derived hash against the real state dir. Since the hash is unique per TempDir, a false-negative (hash coincidentally appears elsewhere) is essentially impossible.

Struct literal updates (environment.rs:485,520, template.rs:148,183)

Mechanical state_dir: None additions to existing struct literals in test code. Correct, no concerns.

Production callers unaffected (confirmed)

Grepped all HookContext::new() call sites in src/commands/. None call .with_state_dir(), so they all get None, falling through to daft_state_dir() — correct production behavior.

state_base clone in yaml_executor (mod.rs:163-166)

let state_base = match &ctx.state_dir {
    Some(p) => p.clone(),
    None => crate::daft_state_dir()?,
};

Idiomatic. The clone is a one-shot PathBuf and avoids lifetime complexity. Fine.

---

Verdict

Approve. The fix closes the leak cleanly, the regression test actually catches the regression (the PR confirms it fails when reverted), and no production paths are changed. The pub vs pub(crate) point on for_repo_in is the only thing worth a second look, but it's not blocking given the binary-crate context.