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
…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>
![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]
…(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>
|
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 |
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]
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