feat(hooks): worktree-collision guardrail (isolation:worktree guard + git-worktree-add warn rule) - #46
feat(hooks): worktree-collision guardrail (isolation:worktree guard + git-worktree-add warn rule)#46lapc506 wants to merge 2 commits into
Conversation
![dojo-code-reviewer[bot]](https://avatars.githubusercontent.com/in/3092887?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Changes Requested
Changes requested — 1 blocker, 1 P3. Confidence: 1.00/5.00.
🔴 P1 — Blockers
hooks/pre-task-worktree-isolation-guard.sh:67— 🔴 P1 (blocker) — A time-of-check to time-of-use (TOCTOU) race condition in lock reclamation can lead to a newly-created lock from a racing parallel process being deleted. This allows both processes to pass, defeating the primary parallel-spawn guardrail of the PR.
[pass 1]
🔵 P3 — Minor
hooks/pre-task-worktree-isolation-guard.sh:45— 🔵 P3 (minor) — Validate that WINDOW_SECONDS is a positive integer to prevent runtime failures in bash arithmetic under set -u if a user provides an invalid custom MNM_WORKTREE_GUARD_WINDOW environment variable.
[pass 1]
Total findings: 1 security, 1 business context (2 total)
| # window are treated as a racing batch. The SAFE multi-worktree pattern | ||
| # (pre-create + git -C, Rule 2) does NOT use isolation:worktree, so it never | ||
| # trips this guard. | ||
| WINDOW_SECONDS="${MNM_WORKTREE_GUARD_WINDOW:-90}" |
There was a problem hiding this comment.
🔵 P3 (minor) — Validate that WINDOW_SECONDS is a positive integer to prevent runtime failures in bash arithmetic under set -u if a user provides an invalid custom MNM_WORKTREE_GUARD_WINDOW environment variable.
[pass 1]
| now="$(date +%s 2>/dev/null || echo 0)" | ||
| [[ "$now" =~ ^[0-9]+$ ]] || exit 0 # fail-open: no clock | ||
|
|
||
| # Reclaim a stale lock from a prior (already-provisioned) batch. |
There was a problem hiding this comment.
🔴 P1 (blocker) — A time-of-check to time-of-use (TOCTOU) race condition in lock reclamation can lead to a newly-created lock from a racing parallel process being deleted. This allows both processes to pass, defeating the primary parallel-spawn guardrail of the PR.
[pass 1]
|
Both findings addressed in
Re-tested: non-worktree→pass, 1st→pass, 2nd-in-window→block, stale→pass (dir NOT deleted), invalid-window→fallback-90 block. @dojo-code-reviewer review |
…(review #46) P1: the rm -rf reclamation between the staleness check and mkdir could delete a racing parallel process's freshly-created lock, letting both spawns pass. The lock dir is now NEVER deleted — mkdir is the sole atomic gate and staleness is decided by the timestamp (stale → allow + roll the window; fresh → block). P3: fall back to the default window if MNM_WORKTREE_GUARD_WINDOW is not a positive integer (avoids set -u arithmetic failure). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Approved
Approved — 0 blockers, 2 P3. Confidence: 4.60/5.00.
🔵 P3 — Minor
hooks/pre-task-worktree-isolation-guard.sh:77— 🔵 P3 (minor) — Writing directly totsvia redirection truncate (>) can cause a racing process (readingtsconcurrently on line 88) to read an empty file if the scheduler interleaves between the truncation and the write. Using a temp file and an atomicmvguarantees a safe, race-free, non-blocking read.
[pass 1]
hooks/pre-task-worktree-isolation-guard.sh:92— 🔵 P3 (minor) — Writing directly totsvia redirection truncate (>) can cause a racing process (readingtsconcurrently on line 88) to read an empty file if the scheduler interleaves between the truncation and the write. Using a temp file and an atomicmvguarantees a safe, race-free, non-blocking read.
[pass 1]
Total findings: 2 business context (2 total)
| # existence, so there is no check-then-rm (TOCTOU) window in which a racing | ||
| # process's freshly-created lock could be removed. | ||
| if mkdir "$LOCK_DIR" 2>/dev/null; then | ||
| printf '%s' "$now" > "$LOCK_DIR/ts" 2>/dev/null || true |
There was a problem hiding this comment.
🔵 P3 (minor) — Writing directly to ts via redirection truncate (>) can cause a racing process (reading ts concurrently on line 88) to read an empty file if the scheduler interleaves between the truncation and the write. Using a temp file and an atomic mv guarantees a safe, race-free, non-blocking read.
[pass 1]
| held="$(cat "$LOCK_DIR/ts" 2>/dev/null || echo "$now")" | ||
| [[ "$held" =~ ^[0-9]+$ ]] || held="$now" | ||
| age=$(( now - held )) | ||
| if (( age >= WINDOW_SECONDS )); then |
There was a problem hiding this comment.
🔵 P3 (minor) — Writing directly to ts via redirection truncate (>) can cause a racing process (reading ts concurrently on line 88) to read an empty file if the scheduler interleaves between the truncation and the write. Using a temp file and an atomic mv guarantees a safe, race-free, non-blocking read.
[pass 1]
There was a problem hiding this comment.
✅ Approved
Approved — 0 blockers, 1 P3. Confidence: 4.80/5.00.
🔵 P3 — Minor
hooks/pre-task-worktree-isolation-guard.sh:62— 🔵 P3 (minor) — Make the lock root user-specific to prevent potential permission collisions on multi-user development machines. If User A runs the hook first, they will create the shared/tmp/mnm-worktree-spawn-guarddirectory. Depending on their system umask, this directory might be created with restrictive permissions (like700). If User B then runs a task on the same machine, they will not have write access to that shared directory, causing their spawns to fall back to the fail-closed path and get blocked. Appending the username (using${USER:-shared}) ensures isolated lock directories for different users on the same host.
[pass 1]
Total findings: 1 business context (1 total)
| SESSION_ID="$(printf '%s' "$INPUT_RAW" | jq -r '.session_id // "default"' 2>/dev/null || echo default)" | ||
| SESSION_ID="${SESSION_ID//[^A-Za-z0-9_.-]/_}" # sanitize for use in a path | ||
| LABEL="$(printf '%s' "$INPUT_RAW" | jq -r '.tool_input.name // .tool_input.description // "agent"' 2>/dev/null || echo agent)" | ||
|
|
There was a problem hiding this comment.
🔵 P3 (minor) — Make the lock root user-specific to prevent potential permission collisions on multi-user development machines. If User A runs the hook first, they will create the shared /tmp/mnm-worktree-spawn-guard directory. Depending on their system umask, this directory might be created with restrictive permissions (like 700). If User B then runs a task on the same machine, they will not have write access to that shared directory, causing their spawns to fall back to the fail-closed path and get blocked. Appending the username (using ${USER:-shared}) ensures isolated lock directories for different users on the same host.
[pass 1]
…orktree spawns + git-worktree-add warn rule PreToolUse guard (Task/Agent) blocks a 2nd isolation:worktree spawn in the provisioning window (atomic mkdir lock for same-message parallel spawns); git-worktree-add-discipline warn rule nudges toward dedicated-worktree + git -C; references/agent-worktree-orchestration.md documents the pattern + forward-fix recovery. Genericized warn-localhost example host -> dev.example.com. No version bump (coordinate with the audit-engine release). Created by Claude Code on behalf of @lapc506. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(review #46) P1: the rm -rf reclamation between the staleness check and mkdir could delete a racing parallel process's freshly-created lock, letting both spawns pass. The lock dir is now NEVER deleted — mkdir is the sole atomic gate and staleness is decided by the timestamp (stale → allow + roll the window; fresh → block). P3: fall back to the default window if MNM_WORKTREE_GUARD_WINDOW is not a positive integer (avoids set -u arithmetic failure). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2d26dd5 to
c41e6c8
Compare
There was a problem hiding this comment.
💬 Review Comments
Comments — 0 blockers, 1 P2. Confidence: 3.80/5.00.
Walkthrough
main branch directly. Per standard GitFlow conventions, feature branches should target develop (feature → develop → main). This warning is informational only; the following review verdict is based solely on the technical quality, design, and safety of the implementation.
Walkthrough
This PR introduces a robust double-sided guardrail mechanism to prevent sibling-recheckout collisions (parallel agents commingling worktrees and producing entangled commits) when orchestrating parallel subagents in standard Git repositories:
- Blocking PreToolUse Hook: Adds
hooks/pre-task-worktree-isolation-guard.shwhich dynamically interceptsTaskorAgenttool invocations. It enforces a safety window (default 90s) between consecutiveisolation:worktreespawns under the same session by utilizing atomicmkdirlock checking. - Warn Rule: Appends
git-worktree-add-disciplinetohooks/rules/rules.yaml(and updates the compiledrules.json), nudging developers and agents away from baregit worktree addcommands in favor of pre-created, explicit-C <abs-path>pins. - Reference Documentation: Introduces
references/agent-worktree-orchestration.mddetailing the failure modes, safety rules, and emergency recovery strategies. - Generalization: Updates the example staging domain from
dev.dojocoding.ioto a genericdev.example.cominwarn-localhost-in-pr-body.
Reviewed Files
hooks/hooks.jsonhooks/pre-task-worktree-isolation-guard.shhooks/rules/rules.yaml&hooks/rules/rules.jsonreferences/agent-worktree-orchestration.md
Safety Rationale
The design prioritizes fail-open behavior: missing jq or an unwritable lock root will not block normal execution. Furthermore, CLAUDE_DISABLE_PLUGIN_HOOKS=1 acts as a quick-escape toggle. These measures guarantee that any configuration issues within the safety harness will not block core developer workflows.
Minor Suggestions (P3)
- Documentation Inconsistency: In
hooks/rules/README.md(lines 175 and 217), there are still references to the old staging domaindev.dojocoding.ioinstead ofdev.example.com. Consider updating these references to match the public-toolkit agnostic naming introduced in this PR.
Verdict
Commented — 0 blockers, 1 P2.
🟡 P2 — Major
hooks/pre-task-worktree-isolation-guard.sh:63— 🟡 P2 (major) — In multi-user environments (such as shared development servers, multi-tenant machines, or shared CI runners), using a single global lock root/tmp/mnm-worktree-spawn-guardwill cause permission clashes. The first user who triggers the hook will create this directory with standard0755permissions, preventing any other user from writing/creating files within it.
Since mkdir -p returns 0 if the directory already exists (even if it is owned by someone else and not writable), the script will bypass the || exit 0 escape hatch on line 64, fail to create the user's specific lock directory on line 65, fallback to a relative age of 0 on line 71, and then block execution with exit 2. This directly violates the hook's stated design contract to fail-open under unwritable lock configurations.
To resolve this, append the current user's UID (using the built-in ${UID} with standard fallback) to the lock root path, ensuring isolated, user-specific lock roots.
[pass 1]
Total findings: 1 compliance (1 total)
| SESSION_ID="${SESSION_ID//[^A-Za-z0-9_.-]/_}" # sanitize for use in a path | ||
| LABEL="$(printf '%s' "$INPUT_RAW" | jq -r '.tool_input.name // .tool_input.description // "agent"' 2>/dev/null || echo agent)" | ||
|
|
||
| LOCK_ROOT="${TMPDIR:-/tmp}/mnm-worktree-spawn-guard" |
There was a problem hiding this comment.
🟡 P2 (major) — In multi-user environments (such as shared development servers, multi-tenant machines, or shared CI runners), using a single global lock root /tmp/mnm-worktree-spawn-guard will cause permission clashes. The first user who triggers the hook will create this directory with standard 0755 permissions, preventing any other user from writing/creating files within it.
Since mkdir -p returns 0 if the directory already exists (even if it is owned by someone else and not writable), the script will bypass the || exit 0 escape hatch on line 64, fail to create the user's specific lock directory on line 65, fallback to a relative age of 0 on line 71, and then block execution with exit 2. This directly violates the hook's stated design contract to fail-open under unwritable lock configurations.
To resolve this, append the current user's UID (using the built-in ${UID} with standard fallback) to the lock root path, ensuring isolated, user-specific lock roots.
[pass 1]
Measured on origin/main @ ee0ba47: the three version files all read 1.36.0, and `gh pr diff 55 | grep -E '^\+.*"version"'` shows #55 already bumps to 1.37.0 in package.json, plugin.json and marketplace.json. Taking 1.37.0 here collides with it in all three. The same probe over #52, #46, #44 and #41 returns nothing, so 1.38.0 is free. Skipping a version costs nothing; colliding does. Minor and not major, decided by reading. A skill auto-activates on its `description` rather than being invoked by name like a command, so a renamed `name:` changes no call site. The reference search returns nothing: grep -rniI "rebase.advisor" . --exclude-dir=node_modules \ --exclude-dir=.git --exclude=CHANGELOG.md -> no hits That negative is real and not a broken search -- the same grep for `spike-recommend` returns 10 files, so cross-references of this shape are found when they exist. One surface does break and is stated rather than folded in: a user who typed `/make-no-mistakes:rebase-advisor` explicitly (README:137 documents that skills can be invoked that way) now gets an unknown skill. It fails loudly, the replacement is one row away in the same table, and the installer prunes the old file rather than leaving both live. The `[1.37.0]:` CHANGELOG reference slot is deliberately left for #55 to fill. Suite: 337/337 hooks, 60/60 vitest. Created by Claude Code on behalf of @lapc506
…(v1.38.0) (#56) * feat(skills): sync-advisor — measure the drift before naming the fix (v1.37.0) Renames `rebase-advisor` to `sync-advisor` and turns a blind router into an advisor that measures. The old skill was 43 lines and measured nothing. Its step 1 was "Confirm the user wants a full team sync" -- a question back to the user about something three git commands answer. Its description then over-routed: it triggered on "align with develop" and "branches are behind", both of which are `git pull`, and sent them to /make-no-mistakes:rebase, which stashes every worktree, rebases every local branch and auto-merges PRs. Between `git pull` and that the toolkit offered nothing, and nothing read-only at all. Six read-only predicates now run before anything is named: distance, fast-forward possible, dirty tree split by stage, untracked files the ref already tracks, worktrees behind, and branches with unpushed commits. The fifth is the threshold between a plain pull and the team command. The fourth is the one nothing else reports, and it was verified on a throwaway pair of repos rather than asserted. An untracked local file at a path the ref tracks aborts the pull outright while being invisible everywhere else: `git status` shows a plain `?? newfile.txt`, distance reports a clean 0 ahead 1 behind, and `merge-base --is-ancestor` says a fast-forward is possible. The pull then exits 1 with "The following untracked working tree files would be overwritten by merge ... Please move or remove them before you merge" -- the message names the user's own file and offers deletion as the remedy, which is the one irreversible move available. The skill reports these by name and recommends copying them out of the repo, never deleting them. Both controls were run: the pipeline printed nothing before the collision existed and named the file after. It never acts. Every fix is printed for the user to run. The single write is `git fetch origin --quiet`, which touches remote-tracking refs and nothing else, and the skill says so out loud -- without it every measurement is taken against a stale origin/<base> and reports a drift that stopped being true days ago, which is the failure this skill exists to catch. Adds `syncAdvisor.governedPaths` to make-no-mistakes.config.json: the paths whose changes get reported by name, turning "you are 12 behind" into "three hooks changed, two of them fix defects you may be looking at right now". No default -- with the key unset the skill drops the consequence line rather than falling back to a built-in list, which would be wrong in every repo but the one it was copied from and would read as measured. commands/rebase.md is untouched and stays a real destination. What changed is who decides when it applies. Origin (2026-07-31, as reported): a developer filed two bug reports against a hook with clean reproductions. One was a real defect; the other described behaviour fixed days earlier against a stale checkout, and nothing in the report separated them. Suite: 337/337 hooks, 60/60 vitest. Created by Claude Code on behalf of @lapc506 * chore(release): take 1.38.0 instead of 1.37.0 — #55 owns 1.37.0 Measured on origin/main @ ee0ba47: the three version files all read 1.36.0, and `gh pr diff 55 | grep -E '^\+.*"version"'` shows #55 already bumps to 1.37.0 in package.json, plugin.json and marketplace.json. Taking 1.37.0 here collides with it in all three. The same probe over #52, #46, #44 and #41 returns nothing, so 1.38.0 is free. Skipping a version costs nothing; colliding does. Minor and not major, decided by reading. A skill auto-activates on its `description` rather than being invoked by name like a command, so a renamed `name:` changes no call site. The reference search returns nothing: grep -rniI "rebase.advisor" . --exclude-dir=node_modules \ --exclude-dir=.git --exclude=CHANGELOG.md -> no hits That negative is real and not a broken search -- the same grep for `spike-recommend` returns 10 files, so cross-references of this shape are found when they exist. One surface does break and is stated rather than folded in: a user who typed `/make-no-mistakes:rebase-advisor` explicitly (README:137 documents that skills can be invoked that way) now gets an unknown skill. It fails loudly, the replacement is one row away in the same table, and the installer prunes the old file rather than leaving both live. The `[1.37.0]:` CHANGELOG reference slot is deliberately left for #55 to fill. Suite: 337/337 hooks, 60/60 vitest. Created by Claude Code on behalf of @lapc506 * fix(sync-advisor): make the collision predicate correct from any subdirectory Reviewer P3 on `ea8c46e` claimed `git ls-tree` "always outputs repository-relative paths" while `git ls-files` is prefix-relative, so the two would fail to match from a subdirectory, and proposed `--full-name` on `ls-files`. Measured: the premise is false and the proposed fix introduces the bug it claims to prevent. From `sub/`, `ls-tree -r --name-only` prints `newfile.txt`, not `sub/newfile.txt` -- it strips the prefix exactly like `ls-files` does, so the original command matched fine and found the collision. Adding `--full-name` alone then makes `ls-files` emit `sub/newfile.txt` against `ls-tree`'s `newfile.txt`, they stop matching, and `comm -12` returns empty -- a clean bill of health for a tree about to abort the pull. But it pointed at a real weakness of a different kind. From a subdirectory both commands are SCOPED to that subtree, so a collision at the repo root is not seen at all. That is scope, not format, and no combination of format flags fixes it: `--full-name` changes how a path prints, never which paths are considered. The pathspec does. Shipped: `--full-name -- :/` on ls-files, `--full-tree` on ls-tree. Four cases run, with the command extracted verbatim from SKILL.md so the test cannot drift from the doc -- negative control from the root and from `sub/` (both empty), positive from the root and from `sub/` with one collision in each location (both list `rootfile.txt` and `sub/newfile.txt`). Suite: 340/340 hooks, 60/60 vitest. Created by Claude Code on behalf of @lapc506 * fix(sync-advisor): resolve the base ref correctly, and in a bare form Reviewer P2 on `00e2c08`, and it is right. Step 0 returned values carrying the remote prefix while every predicate interpolates `origin/$BASE`, so the run died on `origin/origin/develop`: $ git rev-list --left-right --count "HEAD...origin/origin/main" fatal: ambiguous argument … unknown revision exit=128 Fixed by normalising unconditionally (`${BASE#refs/remotes/}`, `${BASE#origin/}`) whichever branch of the resolution produced the value. Verified both branches with a control that must fail: where `origin/HEAD` IS set it returns `origin/main`, normalises to `main`, and `origin/main` resolves; the un-normalised form exits 128. The P2 understated it. Step 0's first command resolved the base from `@{upstream}`, which on a feature branch is that branch's OWN remote copy -- here `origin/andres/sync-advisor`. That answers "am I pushed?", which is predicate 6's question, and would report 0 behind on a branch far behind the real base. Stripping a prefix would have left it pointing at the wrong ref, so the command is gone from base resolution rather than patched. Also measured while there: `git symbolic-ref --short refs/remotes/origin/HEAD` fails outright in this repo (`not a symbolic ref`), so it is documented as a fall-through into the develop/main/master/trunk probe rather than as a step that is expected to succeed. P4 (`--abbrev-ref` and `--symbolic-full-name` redundant) is also correct -- both forms return `origin/andres/sync-advisor` here -- and is moot: that command no longer appears. Suite: 340/340 hooks, 60/60 vitest. Created by Claude Code on behalf of @lapc506
1 participant
Worktree-collision guardrail
Adds the two enforcement layers that prevent the sibling-recheckout collision — parallel
isolation:worktreesubagents commingling in one shared worktree, producing entangled commits recoverable only by forward-fix:hooks/pre-task-worktree-isolation-guard.sh— PreToolUse onTask/Agent. Blocks (exit 2) a 2ndisolation:worktreespawn inside the provisioning window; an atomicmkdirlock handles same-message parallel spawns. Fail-open; honorsCLAUDE_DISABLE_PLUGIN_HOOKS=1. Registered inhooks/hooks.json(newTask|Agentmatcher).git-worktree-add-discipline— warn rule inhooks/rules/rules.yaml(+5 tests), the upstream nudge toward "dedicated worktree +git -C".references/agent-worktree-orchestration.md— the safe pattern + recovery doc.Also: the
warn-localhost-in-pr-bodyrule's example staging host was genericized todev.example.com(public-toolkit agnostic).rules.jsonregenerated in sync withrules.yaml. No version bump / CHANGELOG here — coordinate with the in-flight audit-engine release bump.Created by Claude Code on behalf of @lapc506.
🤖 Generated with Claude Code