feat(git-worktree): auto-trust mise and direnv configs in new worktrees#312
feat(git-worktree): auto-trust mise and direnv configs in new worktrees#312NimbleEngineer21 wants to merge 8 commits intoEveryInc:mainfrom
Conversation
When worktree-manager.sh creates a new worktree, tools like mise and direnv don't recognize the new filesystem path as trusted. This causes hooks (e.g. lefthook pre-commit) that invoke `mise x -- <command>` to hang on an interactive trust prompt that can never be answered. Add a trust_dev_tools() function that runs `mise trust` and `direnv allow` in the new worktree immediately after creation. The fix is completely invisible to developers who don't use these tools (double-guarded by command -v and file existence checks). Follows the existing copy_env_files pattern.
|
@codex review please |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c25d98e791
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
Security: - Only auto-trust configs unchanged from the base branch - Modified configs (e.g., from a PR) are flagged with a warning and manual trust instructions, preventing code execution from untrusted branches Error handling: - Remove 2>/dev/null; show warnings on trust failure - Track attempted vs successful trusts; warn when all attempts fail Comments & docs: - Fix mise comment: trust is directory-scoped, not per-file - Drop inaccurate "silently fail" from function doc - Update SKILL.md feature list and critical section - Fix help text indentation to match existing style
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdb688f81f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
|
@tmchow Addressed the Codex review feedback in bdb688f. The main change: |
…from_branch When from_branch is a PR branch, comparing configs against it would trivially pass since the worktree was just created from that ref. Always compare against the repo's default branch (detected via origin/HEAD, falling back to main) so the safety gate works regardless of which branch the worktree is created from.
tmchow
left a comment
tmchow
left a comment
There was a problem hiding this comment.
Good contribution -- the problem is real and the safety model (unchanged-from-default-branch = safe to auto-trust) is the right call. Two issues in the safety gate need to be fixed before this can merge. Both are small fixes in the same spot.
1. origin/HEAD fallback silently produces an empty string -- when origin/HEAD isn't configured (common with --single-branch clones), sed exits 0 on empty input so the || fallback never fires. default_branch becomes "", and _config_unchanged ends up doing git show ":$file" (index lookup), which can silently bypass the safety gate.
2. Comparison uses local branch ref instead of remote tracking ref -- git show "main:$file" reads from the local main, which create_worktree doesn't pull when from_branch is a PR branch. Stale local main means an inaccurate comparison.
See inline comment for the fix -- three lines.
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
…ch detection
Address tmchow's review:
1. Without set -o pipefail, failed git symbolic-ref piped through sed
still exits 0, making the || fallback never fire and default_branch
silently become "". Use ${default_branch:-main} for safe fallback.
2. Use origin/$default_branch instead of bare branch name so
_config_unchanged always compares against the remote tracking ref,
not a potentially stale local ref.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fab2fb225
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
P1 fixes: - Fetch origin/$default_branch before comparing so ref is fresh even when from_branch is a PR branch - Pass specific config file to mise trust ($f) instead of bare mise trust, which may not trust .tool-versions repos - Rename from_branch param to base_ref in trust_dev_tools to prevent future contributors from accidentally passing the wrong value P2 fixes: - Drop attempted counter; per-tool warnings already distinguish "nothing to do" from "everything failed" - Build manual trust command from skipped array instead of hardcoding both mise trust and direnv allow - Rename wt_path to worktree_path in _config_unchanged for consistency - Collapse double git show into single call with || return 1 P3 fixes: - Document TOCTOU race condition as accepted risk (requires local filesystem write access + timing on single-user dev machines) - Document gitattributes line-ending edge case (mismatch causes false negative which is the safe direction)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bad2a46f3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
…tput 1. Separate git show from git hash-object pipe in _config_unchanged so || return 1 actually fires when the file doesn't exist on the base branch. Without pipefail, the previous pipeline always exited 0 because git hash-object --stdin succeeds on empty input. 2. Join skipped[] entries with ' && ' instead of space-concatenating, so the suggested manual command is valid shell when both mise and direnv configs are skipped. 3. Warn on git fetch failure instead of silent || true, so the user knows the trust comparison may use stale data.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9428d31b0a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
The previous approach captured git show output via command substitution, which strips trailing newlines. This caused the base hash to always differ from the worktree file hash, making auto-trust effectively dead code (every file appeared "modified"). Use git rev-parse to get the blob SHA directly from git's object database. This avoids content round-tripping through shell variables, handles binary files correctly, and is simpler than both the original pipe approach and the separated git show approach.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 369be4cccd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
plugins/compound-engineering/skills/git-worktree/scripts/worktree-manager.sh
Outdated
Show resolved
Hide resolved
When git fetch fails AND the ref doesn't exist locally at all, there is no baseline to compare against. Proceed with stale-but-present refs (warn only), but skip trust entirely when the ref is absent. This fails closed when there is truly no data without penalizing developers who are temporarily offline with a recently-fetched ref.
There was a problem hiding this comment.
@NimbleEngineer21 a few more things to just squeeze in while we are pounding on this.
Stale comment references git show
The _config_unchanged comment says "git show pipes raw bytes" but the code now uses git rev-parse. It will mislead future maintainers. Suggested replacement:
# Note: rev-parse returns the stored blob hash; hash-object on a path applies
# gitattributes filters. A mismatch causes a false negative (trust skipped),
# which is the safe direction.
git fetch stderr silenced with 2>/dev/null
Drop 2>/dev/null from the fetch — --quiet already suppresses normal output. Let actual errors (auth failures, DNS issues) flow through so users can debug trust failures.
Add symlink guard to _config_unchanged
While the current behavior is fail-closed for symlinks, an explicit [[ -L "$worktree_path/$file" ]] && return 1 check makes the intent clear and guards against regressions.
Add non-empty hash guard
Change the comparison to [[ -n "$base_hash" && -n "$worktree_hash" && "$base_hash" == "$worktree_hash" ]]. Costs nothing, eliminates the theoretical empty-string-equals-empty-string case.
Compress TOCTOU comment
Super minor but the 3-line TOCTOU comment raises a concern then immediately dismisses it. Either remove or compress to one line:
# TOCTOU between hash-check and trust is acceptable for local dev use.
2 participants
Summary
trust_dev_tools()function toworktree-manager.shthat automatically runsmise trustanddirenv allowafter creating a new worktreeorigin/HEAD, falling back tomain); modified configs (e.g., from a PR) are flagged for manual reviewProblem
When
worktree-manager.shcreates a new worktree, tools like mise and direnv don't recognize the new filesystem path as trusted. When hooks (e.g. lefthook pre-commit) invokemise x -- <command>, mise opens an interactive trust prompt that hangs indefinitely since hook subprocesses can't forward user input. direnv has the same trust model withdirenv allow.Approach
Follows the existing
copy_env_filespattern: a dedicated function called fromcreate_worktree()immediately after env file copying. Double-guarded bycommand -v(tool installed?) and file existence checks (config present?) -- completely invisible to developers who don't use these tools.Safety model: Before auto-trusting, each config file is hash-compared against the repo's default branch via
_config_unchanged(). The comparison always uses the default branch (notfrom_branch), so passing a PR branch as the base doesn't bypass the safety check. This means:.envrc/.mise.tomlwas modified or added: configs differ -> skipped with a yellow warning listing what was skipped and how to trust manuallymise: trusts
.mise.toml,mise.toml, or.tool-versionsif unchanged from default branchdirenv: allows
.envrcif unchanged from default branchError handling: Trust failures surface warnings (no
2>/dev/null). When all attempts fail, an explicit warning with manual instructions is shown. Attempted vs successful trusts are tracked separately so "nothing to do" is distinguishable from "everything failed."Test evidence
All tests run against a temporary git repo. direnv was tested via a mock script (same
_config_unchangedgate as mise). Trust failures simulated with mock scripts that exit non-zero.✅ Test 1: Auto-trusts unchanged config
Scenario:
.mise.tomlexists on main, worktree created from main (config unchanged).Verified with
mise trust --show:✅ Test 2: Skips modified config with warning
Scenario: Branch modifies
.mise.toml(changes node version, adds python). Worktree created from that branch.✅ Test 3: Silent when no configs present
Scenario: No
.mise.toml,.tool-versions, or.envrcin the repo.No trust output at all -- completely invisible.
✅ Test 4: Help output shows new section
✅ Test 5: Skips newly added config
Scenario: Branch adds
.mise.tomlthat doesn't exist on main.✅ Test 6: direnv allow runs for unchanged .envrc
Scenario: Both
.mise.tomland.envrcexist on main, unchanged. direnv tested via mock script.✅ Test 7: PR branch as from_branch -- still compares against default branch
Scenario: PR branch modifies both
.mise.tomland.envrc. Passed asfrom_branch. Safety gate should still compare againstmain.Both configs correctly skipped despite
from_branchbeing the PR branch.✅ Test 8: Trust failure shows warnings
Scenario: Both
mise trustanddirenv allowfail (simulated via mock scripts that exit non-zero). Configs are unchanged from main.Errors surfaced clearly. Worktree creation still succeeds (trust is non-blocking).
Test plan
.mise.toml(Test 1).mise.tomlwith warning (Test 2).mise.tomlwith warning (Test 5)direnv allowruns for unchanged.envrc(Test 6)from_branchstill compares against default branch (Test 7)