From ec4624af87b23f0f0953a8f013fb3775948566a3 Mon Sep 17 00:00:00 2001 From: Abhigyan Patwari <126312502+abhigyanpatwari@users.noreply.github.com> Date: Wed, 13 May 2026 08:56:27 +0100 Subject: [PATCH] fix(hooks): cap concurrent augment subprocesses (#1486) (#1510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hooks): cap concurrent augment subprocesses to prevent runaway process spawn (#1486) When Claude Code fires PreToolUse hooks for parallel Grep/Glob/Bash tool calls, each invocation spawned its own `gitnexus augment` subprocess — a Node + LadybugDB cold start that holds resources for several seconds. Under heavy parallel search load (issue #1486: 180+ piled-up processes, load avg > 100), these accumulated faster than they completed because nothing capped concurrent in-flight augments. Add a lockfile-based concurrency guard under `<.gitnexus>/.hook-locks/`: each running hook claims a `.lock`, the guard counts live PIDs and prunes stale entries (>30s mtime or pid no longer alive), and bails silently when MAX_INFLIGHT (3) is reached. Augment is best-effort enrichment — missing a few fires under burst load is preferable to melting the system. Applied to all three hook variants that spawn augment: - gitnexus/hooks/claude/gitnexus-hook.cjs (npm-installed Claude hook) - gitnexus-claude-plugin/hooks/gitnexus-hook.js (plugin Claude hook) - gitnexus-cursor-integration/hooks/gitnexus-hook.cjs (Cursor hook) Co-Authored-By: Claude Opus 4.7 (1M context) * fix(hooks): make augment concurrency cap a hard cap via atomic slot files Address Claude's review of #1510. The original count-then-claim guard had a TOCTOU window: N hooks could each read `active < MAX_INFLIGHT` between readdirSync and the per-pid `wx` write and all proceed, briefly exceeding the cap. The PR title's "cap" language overstated this. Replace with fixed-name `slot-0.lock` ... `slot-N.lock` under `.hook-locks/`. `O_CREAT|O_EXCL` on a fixed path is OS-atomic — exactly one process wins each slot, so the cap is hard regardless of burst arrival timing. Each slot file contains the owning PID so stale-takeover still works when a hook crashes without releasing. PID liveness is checked before age (Claude's Finding 3): a slow-but-alive hook is never wrongly evicted. The 30s age window only kicks in to defend against PID reuse on a long-abandoned slot, well above the 7s augment timeout so a healthy run never hits it. Also adds the missing concurrency-guard tests to cursor-hook.test.ts (Claude's Finding 2): source-level wiring + dead-PID reclaim + 3-slots-full bail. Previously only the CJS and Plugin variants had test coverage for the guard; the Cursor variant was validated only by code inspection. Tests: 5726 passing, +9 from baseline (1 hard-cap burst test + 4 source regressions in hooks.test.ts; 3 source + 2 integration in cursor-hook.test.ts). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(hooks): inspect slot mtime + content via single fd (codeql TOCTOU) CodeQL flagged the stale-takeover path in acquireHookSlot as a potential filesystem race (js/file-system-race): statSync(slotPath) followed by readFileSync(slotPath) gives a TOCTOU window where the file could be swapped between the metadata check and the content read. Replace the two separate path-based calls with a single openSync + fstatSync + readSync + closeSync sequence. Both mtime and owner PID now come from the same file descriptor, so the operations are atomic on one inode. No behavioral change beyond closing the race. Applied to all three hook variants (CJS, Plugin, Cursor). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(hooks): distinguish EPERM from ESRCH in PID liveness check Cursor Bugbot caught a contradiction with the stated design: the bare `catch` after `process.kill(owner, 0)` was treating EPERM (process exists but owned by another user) the same as ESRCH (process gone), which would evict a live slot whenever the lock dir straddled user boundaries. Inspect the error code: ESRCH → dead, evict; EPERM → still alive, keep the slot; anything else → assume alive (be conservative under unexpected failure rather than over-evict). Applied to all three hook variants. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(hooks): fail closed when lock dir cannot be created Previously the mkdirSync catch in acquireHookSlot returned `() => {}` (a truthy no-op). The caller checks `if (!release) return;` to skip augment when the guard can't be established — but a truthy no-op slipped through that check and let augment spawn unguarded. On a cross-user shared `.gitnexus/` or read-only filesystem, N concurrent hooks would each take that branch and reintroduce the #1486 fan-out the guard exists to prevent. Return `null` instead so the caller's `if (!release) return;` skips augment cleanly. Augment is best-effort enrichment — skipping it when the guard fails is strictly safer than running unguarded. Also clarify the stale-slot comment: PID-liveness wins for slots younger than HOOK_LOCK_STALE_MS, but age is the final arbiter beyond 30s (PID-reuse defense). The previous wording said "PID-liveness wins over age" without qualifying it, which contradicted the >30s branch. Add source-level regression tests in hooks.test.ts and cursor-hook.test.ts asserting acquireHookSlot returns null (not () => {}) on lock-dir failure. Note in the Cursor test file that the 10-spawner burst test is not duplicated because the algorithm is byte-for-byte identical to the CJS hook and already covered there. Co-Authored-By: Claude Opus 4.7 (1M context) * refactor(hooks): extract lock guard into helper modules Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/04dd20c5-28fd-433a-83cf-ad83fd03fb32 --------- Co-authored-by: Claude Opus 4.7 (1M context) Co-authored-by: Gergő Magyar Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> --- gitnexus-claude-plugin/hooks/gitnexus-hook.js | 9 +- gitnexus-claude-plugin/hooks/hook-lock.js | 119 +++++++ gitnexus-cursor-integration/README.md | 12 +- .../hooks/gitnexus-hook.cjs | 9 +- .../hooks/hook-lock.cjs | 119 +++++++ gitnexus/hooks/claude/gitnexus-hook.cjs | 9 +- gitnexus/hooks/claude/hook-lock.cjs | 119 +++++++ gitnexus/src/cli/setup.ts | 9 + gitnexus/test/unit/cursor-hook.test.ts | 178 +++++++++++ gitnexus/test/unit/hooks.test.ts | 300 ++++++++++++++++++ 10 files changed, 875 insertions(+), 8 deletions(-) create mode 100644 gitnexus-claude-plugin/hooks/hook-lock.js create mode 100644 gitnexus-cursor-integration/hooks/hook-lock.cjs create mode 100644 gitnexus/hooks/claude/hook-lock.cjs diff --git a/gitnexus-claude-plugin/hooks/gitnexus-hook.js b/gitnexus-claude-plugin/hooks/gitnexus-hook.js index 7d8fbfda4..245d34043 100644 --- a/gitnexus-claude-plugin/hooks/gitnexus-hook.js +++ b/gitnexus-claude-plugin/hooks/gitnexus-hook.js @@ -14,6 +14,7 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); +const { acquireHookSlot } = require('./hook-lock.js'); /** * Read JSON input from stdin synchronously. @@ -217,7 +218,8 @@ function sendHookResponse(hookEventName, message) { function handlePreToolUse(input) { const cwd = input.cwd || process.cwd(); if (!path.isAbsolute(cwd)) return; - if (!findGitNexusDir(cwd)) return; + const gitNexusDir = findGitNexusDir(cwd); + if (!gitNexusDir) return; const toolName = input.tool_name || ''; const toolInput = input.tool_input || {}; @@ -227,6 +229,9 @@ function handlePreToolUse(input) { const pattern = extractPattern(toolName, toolInput); if (!pattern || pattern.length < 3) return; + const release = acquireHookSlot(gitNexusDir); + if (!release) return; + let result = ''; try { const child = runGitNexusCli(['augment', '--', pattern], cwd, 7000); @@ -235,6 +240,8 @@ function handlePreToolUse(input) { } } catch { /* graceful failure */ + } finally { + release(); } if (result && result.trim()) { diff --git a/gitnexus-claude-plugin/hooks/hook-lock.js b/gitnexus-claude-plugin/hooks/hook-lock.js new file mode 100644 index 000000000..759856384 --- /dev/null +++ b/gitnexus-claude-plugin/hooks/hook-lock.js @@ -0,0 +1,119 @@ +const fs = require('fs'); +const path = require('path'); + +const HOOK_LOCK_SUBDIR = '.hook-locks'; +const HOOK_LOCK_MAX_INFLIGHT = 3; +const HOOK_LOCK_STALE_MS = 30000; + +function acquireHookSlot(gitNexusDir) { + const lockDir = path.join(gitNexusDir, HOOK_LOCK_SUBDIR); + try { + fs.mkdirSync(lockDir, { recursive: true }); + } catch { + // Cannot create lock dir (read-only fs, cross-user perm denial, out of + // inodes, etc.) — fail closed by returning null. Caller skips augment. + // Fail-open here would let N concurrent hooks all proceed unguarded and + // reintroduce the #1486 fan-out the guard exists to prevent. + return null; + } + + const myPidStr = String(process.pid); + + for (let slot = 0; slot < HOOK_LOCK_MAX_INFLIGHT; slot++) { + const slotPath = path.join(lockDir, `slot-${slot}.lock`); + for (let attempt = 0; attempt < 2; attempt++) { + try { + fs.writeFileSync(slotPath, myPidStr, { flag: 'wx' }); + let released = false; + const release = () => { + if (released) return; + released = true; + try { + // Only unlink if we still own the slot. If we appeared stale and + // another hook took over, the file now belongs to it — leave alone. + const content = fs.readFileSync(slotPath, 'utf-8').trim(); + if (content === myPidStr) fs.unlinkSync(slotPath); + } catch { + /* already removed or unreadable */ + } + }; + process.on('exit', release); + return release; + } catch { + // Slot exists. Decide whether to take it over. + // Open once and inspect mtime + content via the same fd so there's + // no TOCTOU between the metadata check and the content read + // (codeql js/file-system-race). + let fd; + try { + fd = fs.openSync(slotPath, 'r'); + } catch { + continue; // Vanished between EEXIST and open — retry this slot. + } + let isLive = false; + let mtimeMs = Date.now(); + try { + mtimeMs = fs.fstatSync(fd).mtimeMs; + const buf = Buffer.alloc(32); + const n = fs.readSync(fd, buf, 0, 32, 0); + const ownerStr = buf.slice(0, n).toString('utf-8').trim(); + if (ownerStr === '') { + // Owner created the file but hasn't written its PID yet. The + // wx open+write window is microseconds; give it the benefit + // of the doubt and treat as live. + isLive = true; + } else { + const owner = Number.parseInt(ownerStr, 10); + if (Number.isFinite(owner) && owner > 0) { + try { + process.kill(owner, 0); + isLive = true; + } catch (e) { + // ESRCH = process gone → treat as dead. EPERM = process exists + // but owned by another user (cross-user lock dir) → still alive, + // keep the slot. Anything else: be conservative, assume alive. + if (e && e.code === 'ESRCH') { + isLive = false; + } else { + isLive = true; + } + } + } + } + } catch { + /* unreadable — treat as dead */ + } finally { + try { + fs.closeSync(fd); + } catch { + /* already closed */ + } + } + // For slots younger than HOOK_LOCK_STALE_MS, PID-liveness wins — + // a slow-but-alive hook is never wrongly evicted. For older slots, + // age is the final arbiter as a defense against PID reuse on long- + // abandoned slots. 30s >> the 7s augment timeout, so a healthy run + // never crosses this threshold. + if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) { + isLive = false; + } + if (isLive) break; // Try the next slot. + try { + fs.unlinkSync(slotPath); + } catch { + /* another hook beat us to it — retry will hit EEXIST */ + } + // Loop and retry this slot. + } + } + } + + return null; +} + +module.exports = { + HOOK_LOCK_SUBDIR, + HOOK_LOCK_MAX_INFLIGHT, + HOOK_LOCK_STALE_MS, + acquireHookSlot, +}; diff --git a/gitnexus-cursor-integration/README.md b/gitnexus-cursor-integration/README.md index 0da8b1981..6545bacec 100644 --- a/gitnexus-cursor-integration/README.md +++ b/gitnexus-cursor-integration/README.md @@ -10,20 +10,21 @@ Static config that adds GitNexus knowledge-graph augmentation and skill files to | ------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- | | **MCP** | `gitnexus` MCP server with 16 tools (`query`, `context`, `impact`, `detect_changes`, `rename`, …) | `npx gitnexus setup` writes `~/.cursor/mcp.json` automatically. | | **Skills** | `/gitnexus-exploring`, `/gitnexus-debugging`, `/gitnexus-impact-analysis`, `/gitnexus-refactoring`, `/gitnexus-pr-review` markdown skills | `npx gitnexus setup` copies them to `~/.cursor/skills/gitnexus/`. | -| **Hooks** _(this README)_ | `postToolUse` hook that enriches `Shell` / `Read` / `Grep` tool calls with graph context — same augmentation Claude Code gets | **Manual** — copy the two files described below into your project's `.cursor/`. | +| **Hooks** _(this README)_ | `postToolUse` hook that enriches `Shell` / `Read` / `Grep` tool calls with graph context — same augmentation Claude Code gets | **Manual** — copy the files described below into your project's `.cursor/`. | ## Hook install Cursor 2.4+ reads `.cursor/hooks.json` from the project root and runs hook commands with the project root as the working directory ([docs](https://cursor.com/docs/agent/hooks)). -From this repo's `gitnexus-cursor-integration/hooks/`, copy the two files into your **project root**: +From this repo's `gitnexus-cursor-integration/hooks/`, copy the files below into your **project root**: ```text / ├── .cursor/ │ └── hooks.json ← from gitnexus-cursor-integration/hooks/hooks.json └── hooks/ - └── gitnexus-hook.cjs ← from gitnexus-cursor-integration/hooks/gitnexus-hook.cjs + ├── gitnexus-hook.cjs ← from gitnexus-cursor-integration/hooks/gitnexus-hook.cjs + └── hook-lock.cjs ← from gitnexus-cursor-integration/hooks/hook-lock.cjs ``` Equivalent shell commands (run from your project root, with `$GITNEXUS_REPO` pointing at a clone of this repo): @@ -32,6 +33,7 @@ Equivalent shell commands (run from your project root, with `$GITNEXUS_REPO` poi mkdir -p .cursor hooks cp "$GITNEXUS_REPO/gitnexus-cursor-integration/hooks/hooks.json" .cursor/hooks.json cp "$GITNEXUS_REPO/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs" hooks/gitnexus-hook.cjs +cp "$GITNEXUS_REPO/gitnexus-cursor-integration/hooks/hook-lock.cjs" hooks/hook-lock.cjs ``` If you already have a `.cursor/hooks.json`, merge the `hooks.postToolUse` array rather than overwriting. @@ -49,7 +51,7 @@ If you already have a `.cursor/hooks.json`, merge the `hooks.postToolUse` array | -------------------------------------------------------------------- | ------------------------------ | | `~/.cursor/mcp.json` | ✅ | | `~/.cursor/skills/gitnexus/*` | ✅ | -| `/.cursor/hooks.json` + `/hooks/gitnexus-hook.cjs` | ❌ — copy manually (see above) | +| `/.cursor/hooks.json` + `/hooks/gitnexus-hook.cjs` + `/hooks/hook-lock.cjs` | ❌ — copy manually (see above) | Hook install is per-project (Cursor scopes hooks to a project root); skills and MCP config are global. @@ -84,6 +86,6 @@ Empty stdout means "no augmentation, continue normally" — the hook never block ## Troubleshooting -- **Nothing happens** — Confirm Cursor is on 2.4+ and the project root has both `.cursor/hooks.json` and the script at `hooks/gitnexus-hook.cjs`. Then `npx gitnexus list` to confirm the project is indexed. +- **Nothing happens** — Confirm Cursor is on 2.4+ and the project root has `.cursor/hooks.json` plus both hook files at `hooks/gitnexus-hook.cjs` and `hooks/hook-lock.cjs`. Then `npx gitnexus list` to confirm the project is indexed. - **`gitnexus` not found** — The hook prefers a locally-resolvable `gitnexus/dist/cli/index.js` and falls back to `npx -y gitnexus`. Install globally with `npm i -g gitnexus` to skip the npx cold-start latency. - **Wrong pattern extracted** — Set `GITNEXUS_DEBUG=1` and run a tool call. The raw stdin payload is logged to stderr; use it to confirm Cursor's actual `tool_input` field names against the table above. If they differ, file an issue with the captured payload. diff --git a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs index 0ea336619..74c5587b3 100644 --- a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs +++ b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs @@ -18,6 +18,7 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); +const { acquireHookSlot } = require('./hook-lock.cjs'); function readInput() { try { @@ -227,7 +228,8 @@ function main() { } const cwd = input.cwd || process.cwd(); if (!path.isAbsolute(cwd)) return; - if (!findGitNexusDir(cwd)) return; + const gitNexusDir = findGitNexusDir(cwd); + if (!gitNexusDir) return; const toolName = input.tool_name || ''; const toolInput = input.tool_input || {}; @@ -235,6 +237,9 @@ function main() { const pattern = extractPattern(toolName, toolInput); if (!pattern || pattern.length < 3) return; + const release = acquireHookSlot(gitNexusDir); + if (!release) return; + const cliPath = resolveCliPath(); let result = ''; try { @@ -244,6 +249,8 @@ function main() { } } catch { /* graceful failure */ + } finally { + release(); } if (result && result.trim()) { diff --git a/gitnexus-cursor-integration/hooks/hook-lock.cjs b/gitnexus-cursor-integration/hooks/hook-lock.cjs new file mode 100644 index 000000000..759856384 --- /dev/null +++ b/gitnexus-cursor-integration/hooks/hook-lock.cjs @@ -0,0 +1,119 @@ +const fs = require('fs'); +const path = require('path'); + +const HOOK_LOCK_SUBDIR = '.hook-locks'; +const HOOK_LOCK_MAX_INFLIGHT = 3; +const HOOK_LOCK_STALE_MS = 30000; + +function acquireHookSlot(gitNexusDir) { + const lockDir = path.join(gitNexusDir, HOOK_LOCK_SUBDIR); + try { + fs.mkdirSync(lockDir, { recursive: true }); + } catch { + // Cannot create lock dir (read-only fs, cross-user perm denial, out of + // inodes, etc.) — fail closed by returning null. Caller skips augment. + // Fail-open here would let N concurrent hooks all proceed unguarded and + // reintroduce the #1486 fan-out the guard exists to prevent. + return null; + } + + const myPidStr = String(process.pid); + + for (let slot = 0; slot < HOOK_LOCK_MAX_INFLIGHT; slot++) { + const slotPath = path.join(lockDir, `slot-${slot}.lock`); + for (let attempt = 0; attempt < 2; attempt++) { + try { + fs.writeFileSync(slotPath, myPidStr, { flag: 'wx' }); + let released = false; + const release = () => { + if (released) return; + released = true; + try { + // Only unlink if we still own the slot. If we appeared stale and + // another hook took over, the file now belongs to it — leave alone. + const content = fs.readFileSync(slotPath, 'utf-8').trim(); + if (content === myPidStr) fs.unlinkSync(slotPath); + } catch { + /* already removed or unreadable */ + } + }; + process.on('exit', release); + return release; + } catch { + // Slot exists. Decide whether to take it over. + // Open once and inspect mtime + content via the same fd so there's + // no TOCTOU between the metadata check and the content read + // (codeql js/file-system-race). + let fd; + try { + fd = fs.openSync(slotPath, 'r'); + } catch { + continue; // Vanished between EEXIST and open — retry this slot. + } + let isLive = false; + let mtimeMs = Date.now(); + try { + mtimeMs = fs.fstatSync(fd).mtimeMs; + const buf = Buffer.alloc(32); + const n = fs.readSync(fd, buf, 0, 32, 0); + const ownerStr = buf.slice(0, n).toString('utf-8').trim(); + if (ownerStr === '') { + // Owner created the file but hasn't written its PID yet. The + // wx open+write window is microseconds; give it the benefit + // of the doubt and treat as live. + isLive = true; + } else { + const owner = Number.parseInt(ownerStr, 10); + if (Number.isFinite(owner) && owner > 0) { + try { + process.kill(owner, 0); + isLive = true; + } catch (e) { + // ESRCH = process gone → treat as dead. EPERM = process exists + // but owned by another user (cross-user lock dir) → still alive, + // keep the slot. Anything else: be conservative, assume alive. + if (e && e.code === 'ESRCH') { + isLive = false; + } else { + isLive = true; + } + } + } + } + } catch { + /* unreadable — treat as dead */ + } finally { + try { + fs.closeSync(fd); + } catch { + /* already closed */ + } + } + // For slots younger than HOOK_LOCK_STALE_MS, PID-liveness wins — + // a slow-but-alive hook is never wrongly evicted. For older slots, + // age is the final arbiter as a defense against PID reuse on long- + // abandoned slots. 30s >> the 7s augment timeout, so a healthy run + // never crosses this threshold. + if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) { + isLive = false; + } + if (isLive) break; // Try the next slot. + try { + fs.unlinkSync(slotPath); + } catch { + /* another hook beat us to it — retry will hit EEXIST */ + } + // Loop and retry this slot. + } + } + } + + return null; +} + +module.exports = { + HOOK_LOCK_SUBDIR, + HOOK_LOCK_MAX_INFLIGHT, + HOOK_LOCK_STALE_MS, + acquireHookSlot, +}; diff --git a/gitnexus/hooks/claude/gitnexus-hook.cjs b/gitnexus/hooks/claude/gitnexus-hook.cjs index 7bfa150cd..9541fcb50 100755 --- a/gitnexus/hooks/claude/gitnexus-hook.cjs +++ b/gitnexus/hooks/claude/gitnexus-hook.cjs @@ -14,6 +14,7 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); +const { acquireHookSlot } = require('./hook-lock.cjs'); /** * Read JSON input from stdin synchronously. @@ -207,7 +208,8 @@ function runGitNexusCli(cliPath, args, cwd, timeout) { function handlePreToolUse(input) { const cwd = input.cwd || process.cwd(); if (!path.isAbsolute(cwd)) return; - if (!findGitNexusDir(cwd)) return; + const gitNexusDir = findGitNexusDir(cwd); + if (!gitNexusDir) return; const toolName = input.tool_name || ''; const toolInput = input.tool_input || {}; @@ -217,6 +219,9 @@ function handlePreToolUse(input) { const pattern = extractPattern(toolName, toolInput); if (!pattern || pattern.length < 3) return; + const release = acquireHookSlot(gitNexusDir); + if (!release) return; + const cliPath = resolveCliPath(); let result = ''; try { @@ -226,6 +231,8 @@ function handlePreToolUse(input) { } } catch { /* graceful failure */ + } finally { + release(); } if (result && result.trim()) { diff --git a/gitnexus/hooks/claude/hook-lock.cjs b/gitnexus/hooks/claude/hook-lock.cjs new file mode 100644 index 000000000..759856384 --- /dev/null +++ b/gitnexus/hooks/claude/hook-lock.cjs @@ -0,0 +1,119 @@ +const fs = require('fs'); +const path = require('path'); + +const HOOK_LOCK_SUBDIR = '.hook-locks'; +const HOOK_LOCK_MAX_INFLIGHT = 3; +const HOOK_LOCK_STALE_MS = 30000; + +function acquireHookSlot(gitNexusDir) { + const lockDir = path.join(gitNexusDir, HOOK_LOCK_SUBDIR); + try { + fs.mkdirSync(lockDir, { recursive: true }); + } catch { + // Cannot create lock dir (read-only fs, cross-user perm denial, out of + // inodes, etc.) — fail closed by returning null. Caller skips augment. + // Fail-open here would let N concurrent hooks all proceed unguarded and + // reintroduce the #1486 fan-out the guard exists to prevent. + return null; + } + + const myPidStr = String(process.pid); + + for (let slot = 0; slot < HOOK_LOCK_MAX_INFLIGHT; slot++) { + const slotPath = path.join(lockDir, `slot-${slot}.lock`); + for (let attempt = 0; attempt < 2; attempt++) { + try { + fs.writeFileSync(slotPath, myPidStr, { flag: 'wx' }); + let released = false; + const release = () => { + if (released) return; + released = true; + try { + // Only unlink if we still own the slot. If we appeared stale and + // another hook took over, the file now belongs to it — leave alone. + const content = fs.readFileSync(slotPath, 'utf-8').trim(); + if (content === myPidStr) fs.unlinkSync(slotPath); + } catch { + /* already removed or unreadable */ + } + }; + process.on('exit', release); + return release; + } catch { + // Slot exists. Decide whether to take it over. + // Open once and inspect mtime + content via the same fd so there's + // no TOCTOU between the metadata check and the content read + // (codeql js/file-system-race). + let fd; + try { + fd = fs.openSync(slotPath, 'r'); + } catch { + continue; // Vanished between EEXIST and open — retry this slot. + } + let isLive = false; + let mtimeMs = Date.now(); + try { + mtimeMs = fs.fstatSync(fd).mtimeMs; + const buf = Buffer.alloc(32); + const n = fs.readSync(fd, buf, 0, 32, 0); + const ownerStr = buf.slice(0, n).toString('utf-8').trim(); + if (ownerStr === '') { + // Owner created the file but hasn't written its PID yet. The + // wx open+write window is microseconds; give it the benefit + // of the doubt and treat as live. + isLive = true; + } else { + const owner = Number.parseInt(ownerStr, 10); + if (Number.isFinite(owner) && owner > 0) { + try { + process.kill(owner, 0); + isLive = true; + } catch (e) { + // ESRCH = process gone → treat as dead. EPERM = process exists + // but owned by another user (cross-user lock dir) → still alive, + // keep the slot. Anything else: be conservative, assume alive. + if (e && e.code === 'ESRCH') { + isLive = false; + } else { + isLive = true; + } + } + } + } + } catch { + /* unreadable — treat as dead */ + } finally { + try { + fs.closeSync(fd); + } catch { + /* already closed */ + } + } + // For slots younger than HOOK_LOCK_STALE_MS, PID-liveness wins — + // a slow-but-alive hook is never wrongly evicted. For older slots, + // age is the final arbiter as a defense against PID reuse on long- + // abandoned slots. 30s >> the 7s augment timeout, so a healthy run + // never crosses this threshold. + if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) { + isLive = false; + } + if (isLive) break; // Try the next slot. + try { + fs.unlinkSync(slotPath); + } catch { + /* another hook beat us to it — retry will hit EEXIST */ + } + // Loop and retry this slot. + } + } + } + + return null; +} + +module.exports = { + HOOK_LOCK_SUBDIR, + HOOK_LOCK_MAX_INFLIGHT, + HOOK_LOCK_STALE_MS, + acquireHookSlot, +}; diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index af3c4737a..8f52e0f2d 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -364,6 +364,15 @@ async function installClaudeCodeHooks(result: SetupResult): Promise { // Script not found in source — skip } + try { + await fs.copyFile( + path.join(pluginHooksPath, 'hook-lock.cjs'), + path.join(destHooksDir, 'hook-lock.cjs'), + ); + } catch { + // Helper not found in source — skip + } + const hookPath = path.join(destHooksDir, 'gitnexus-hook.cjs').replace(/\\/g, '/'); // Escape backslashes FIRST, then quotes (CodeQL js/incomplete-sanitization). // The previous shape `replace(/"/g, '\\"')` alone would let `path\with"quote` diff --git a/gitnexus/test/unit/cursor-hook.test.ts b/gitnexus/test/unit/cursor-hook.test.ts index f0d875dee..e64979895 100644 --- a/gitnexus/test/unit/cursor-hook.test.ts +++ b/gitnexus/test/unit/cursor-hook.test.ts @@ -35,6 +35,15 @@ const CURSOR_HOOK = path.resolve( 'hooks', 'gitnexus-hook.cjs', ); +const CURSOR_HOOK_LOCK = path.resolve( + __dirname, + '..', + '..', + '..', + 'gitnexus-cursor-integration', + 'hooks', + 'hook-lock.cjs', +); const CURSOR_HOOKS_JSON = path.resolve( __dirname, '..', @@ -60,16 +69,35 @@ function parseCursorOutput(stdout: string): { additional_context?: string } | nu // ─── Test fixtures ────────────────────────────────────────────────── let tmpDir: string; +// Separate fixture for the concurrency guard tests: this one has a real +// `.gitnexus/` so the hook reaches acquireHookSlot. The base tmpDir above +// deliberately has no .gitnexus so unrelated early-exit tests stay cheap. +let guardTmpDir: string; +let guardGitNexusDir: string; beforeAll(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-cursor-hook-test-')); spawnSync('git', ['init'], { cwd: tmpDir, stdio: 'pipe' }); spawnSync('git', ['config', 'user.email', 'test@test.com'], { cwd: tmpDir, stdio: 'pipe' }); spawnSync('git', ['config', 'user.name', 'Test'], { cwd: tmpDir, stdio: 'pipe' }); + + guardTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-cursor-hook-guard-')); + guardGitNexusDir = path.join(guardTmpDir, '.gitnexus'); + fs.mkdirSync(guardGitNexusDir, { recursive: true }); + spawnSync('git', ['init'], { cwd: guardTmpDir, stdio: 'pipe' }); + spawnSync('git', ['config', 'user.email', 'test@test.com'], { + cwd: guardTmpDir, + stdio: 'pipe', + }); + spawnSync('git', ['config', 'user.name', 'Test'], { cwd: guardTmpDir, stdio: 'pipe' }); + fs.writeFileSync(path.join(guardTmpDir, 'dummy.txt'), 'hello'); + spawnSync('git', ['add', '.'], { cwd: guardTmpDir, stdio: 'pipe' }); + spawnSync('git', ['commit', '-m', 'init'], { cwd: guardTmpDir, stdio: 'pipe' }); }); afterAll(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); + fs.rmSync(guardTmpDir, { recursive: true, force: true }); }); // ─── Manifest + hook file presence ─────────────────────────────────── @@ -375,6 +403,155 @@ describe('Cursor hook debug logging', () => { }); }); +// ─── Source code regression: concurrency guard (#1486) ───────────── + +describe('Cursor hook concurrency guard', () => { + const source = fs.readFileSync(CURSOR_HOOK, 'utf-8'); + const lockSource = fs.readFileSync(CURSOR_HOOK_LOCK, 'utf-8'); + + it('loads acquireHookSlot helper module', () => { + expect(source).toContain('acquireHookSlot'); + expect(source).toContain('hook-lock.cjs'); + }); + + it('helper defines acquireHookSlot with MAX_INFLIGHT constant', () => { + expect(lockSource).toContain('function acquireHookSlot'); + expect(lockSource).toContain('HOOK_LOCK_MAX_INFLIGHT'); + }); + + it('calls acquireHookSlot in main() and releases via finally', () => { + // The Cursor hook uses a flat main() dispatcher rather than a separate + // handlePreToolUse — assert the guard call + finally release wiring is + // present so a future refactor cannot accidentally skip it. + expect(source).toContain('acquireHookSlot('); + expect(source).toMatch(/finally\s*\{[^}]*release\(\)/s); + }); + + it('uses atomic fixed-name slot files (hard cap, not soft TOCTOU cap)', () => { + expect(lockSource).toMatch(/slot-\$\{slot\}\.lock|`slot-/); + const slotFn = lockSource.slice( + lockSource.indexOf('function acquireHookSlot'), + lockSource.indexOf('function', lockSource.indexOf('function acquireHookSlot') + 1), + ); + expect(slotFn).not.toContain('readdirSync'); + }); + + it('fails closed when lock dir cannot be created', () => { + // Regression: see hooks.test.ts. The mkdirSync catch must return null + // (skip augment) rather than `() => {}` (proceed unguarded), so that + // a read-only or cross-user `.gitnexus/` cannot reintroduce #1486. + const slotFn = lockSource.slice( + lockSource.indexOf('function acquireHookSlot'), + lockSource.indexOf('function', lockSource.indexOf('function acquireHookSlot') + 1), + ); + const mkdirCatch = slotFn.slice( + slotFn.indexOf('fs.mkdirSync(lockDir'), + slotFn.indexOf('const myPidStr'), + ); + expect(mkdirCatch).toContain('return null'); + expect(mkdirCatch).not.toMatch(/return\s*\(\s*\)\s*=>\s*\{\s*\}/); + }); + + // Note: the 10-concurrent-spawner burst test that validates `wx` + // (O_CREAT|O_EXCL) under simultaneous contention lives in + // hooks.test.ts. The Cursor hook uses byte-for-byte the same + // acquireHookSlot, so duplicating the burst test here would only test + // the OS primitive, not Cursor-specific wiring. The source-level checks + // above guarantee the Cursor hook keeps calling that same algorithm. +}); + +// ─── Integration: concurrency guard skips when slots are full ────── + +describe('Cursor hook concurrency guard (integration)', () => { + it('exits silently when all MAX_INFLIGHT slots hold live pids', async () => { + const { spawn } = await import('child_process'); + const lockDir = path.join(guardGitNexusDir, '.hook-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + + const sleepers = [0, 1, 2].map(() => + spawn(process.execPath, ['-e', 'setTimeout(()=>{},60000)'], { + stdio: 'ignore', + detached: false, + }), + ); + const writtenLocks: string[] = []; + try { + for (let i = 0; i < sleepers.length; i++) { + const p = path.join(lockDir, `slot-${i}.lock`); + fs.writeFileSync(p, String(sleepers[i].pid)); + writtenLocks.push(p); + } + + const result = runHook(CURSOR_HOOK, { + tool_name: 'Grep', + tool_input: { query: 'validateUser' }, + cwd: guardTmpDir, + }); + + expect(result.stdout.trim()).toBe(''); + for (let i = 0; i < sleepers.length; i++) { + const p = path.join(lockDir, `slot-${i}.lock`); + expect(fs.existsSync(p)).toBe(true); + expect(fs.readFileSync(p, 'utf-8').trim()).toBe(String(sleepers[i].pid)); + } + } finally { + for (const child of sleepers) { + try { + child.kill(); + } catch { + /* ignore */ + } + } + for (const p of writtenLocks) { + try { + fs.unlinkSync(p); + } catch { + /* ignore */ + } + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); + + it('reclaims a slot held by a dead pid', () => { + const lockDir = path.join(guardGitNexusDir, '.hook-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + const deadPid = 2_147_483_640; + const stalePath = path.join(lockDir, 'slot-0.lock'); + try { + fs.writeFileSync(stalePath, String(deadPid)); + expect(fs.readFileSync(stalePath, 'utf-8').trim()).toBe(String(deadPid)); + + runHook(CURSOR_HOOK, { + tool_name: 'Grep', + tool_input: { query: 'validateUser' }, + cwd: guardTmpDir, + }); + + // The hook reclaimed and then released slot-0 — either gone (released) + // or no longer owned by the dead pid. + if (fs.existsSync(stalePath)) { + expect(fs.readFileSync(stalePath, 'utf-8').trim()).not.toBe(String(deadPid)); + } + } finally { + try { + fs.unlinkSync(stalePath); + } catch { + /* already pruned */ + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); +}); + // ─── Documented contract behavior (extractPattern via the live hook) ─ describe('Shell quoted-pattern parser limitations (documented)', () => { @@ -429,6 +606,7 @@ describe('Cursor integration install docs', () => { const body = fs.readFileSync(integrationReadme, 'utf-8'); expect(body).toContain('.cursor/hooks.json'); expect(body).toContain('hooks/gitnexus-hook.cjs'); + expect(body).toContain('hooks/hook-lock.cjs'); expect(body).toContain('Hook install'); }); diff --git a/gitnexus/test/unit/hooks.test.ts b/gitnexus/test/unit/hooks.test.ts index da19da002..346ee1ed6 100644 --- a/gitnexus/test/unit/hooks.test.ts +++ b/gitnexus/test/unit/hooks.test.ts @@ -26,6 +26,7 @@ import { runHook, parseHookOutput } from '../utils/hook-test-helpers.js'; // ─── Paths to both hook variants ──────────────────────────────────── const CJS_HOOK = path.resolve(__dirname, '..', '..', 'hooks', 'claude', 'gitnexus-hook.cjs'); +const CJS_HOOK_LOCK = path.resolve(__dirname, '..', '..', 'hooks', 'claude', 'hook-lock.cjs'); const PLUGIN_HOOK = path.resolve( __dirname, '..', @@ -35,6 +36,15 @@ const PLUGIN_HOOK = path.resolve( 'hooks', 'gitnexus-hook.js', ); +const PLUGIN_HOOK_LOCK = path.resolve( + __dirname, + '..', + '..', + '..', + 'gitnexus-claude-plugin', + 'hooks', + 'hook-lock.js', +); // ─── Test fixtures: temporary .gitnexus directory ─────────────────── @@ -294,6 +304,296 @@ describe('Git mutation regex', () => { } }); +// ─── Source code regression: PreToolUse concurrency guard (#1486) ── + +describe('PreToolUse concurrency guard', () => { + for (const [label, hookPath, lockPath] of [ + ['CJS', CJS_HOOK, CJS_HOOK_LOCK], + ['Plugin', PLUGIN_HOOK, PLUGIN_HOOK_LOCK], + ] as const) { + it(`${label} hook loads acquireHookSlot helper`, () => { + const source = fs.readFileSync(hookPath, 'utf-8'); + expect(source).toContain('acquireHookSlot'); + expect(source).toContain('hook-lock'); + }); + + it(`${label} helper defines acquireHookSlot`, () => { + const source = fs.readFileSync(lockPath, 'utf-8'); + expect(source).toContain('function acquireHookSlot'); + expect(source).toContain('HOOK_LOCK_MAX_INFLIGHT'); + }); + + it(`${label} hook calls acquireHookSlot in handlePreToolUse`, () => { + const source = fs.readFileSync(hookPath, 'utf-8'); + const preBody = source.slice( + source.indexOf('function handlePreToolUse'), + source.indexOf('function handlePostToolUse'), + ); + expect(preBody).toContain('acquireHookSlot('); + expect(preBody).toMatch(/release\(\)/); + }); + + it(`${label} hook uses atomic fixed-name slot files (hard cap)`, () => { + // Regression for the TOCTOU soft-cap: an earlier revision counted + // entries then wrote a per-pid lock, which let simultaneous bursts + // exceed MAX_INFLIGHT. The hard-cap version writes to fixed-name + // slot-N.lock paths so O_CREAT|O_EXCL is atomic across processes. + const source = fs.readFileSync(lockPath, 'utf-8'); + expect(source).toMatch(/slot-\$\{slot\}\.lock|`slot-/); + // And no longer reads the lock dir to count active hooks. + const slotFn = source.slice( + source.indexOf('function acquireHookSlot'), + source.indexOf('function', source.indexOf('function acquireHookSlot') + 1), + ); + expect(slotFn).not.toContain('readdirSync'); + }); + + it(`${label} hook fails closed when lock dir cannot be created`, () => { + // Regression: an earlier revision returned `() => {}` (truthy no-op) on + // mkdirSync failure, which left callers — `if (!release) return;` — to + // proceed unguarded and reintroduce the #1486 fan-out on read-only or + // cross-user `.gitnexus/` setups. The guard must fail closed (null). + const source = fs.readFileSync(lockPath, 'utf-8'); + const slotFn = source.slice( + source.indexOf('function acquireHookSlot'), + source.indexOf('function', source.indexOf('function acquireHookSlot') + 1), + ); + const mkdirCatch = slotFn.slice( + slotFn.indexOf('fs.mkdirSync(lockDir'), + slotFn.indexOf('const myPidStr'), + ); + expect(mkdirCatch).toContain('return null'); + expect(mkdirCatch).not.toMatch(/return\s*\(\s*\)\s*=>\s*\{\s*\}/); + }); + } +}); + +// ─── Integration: concurrency guard skips when slots are full ────── + +describe('PreToolUse concurrency guard (integration)', () => { + for (const [label, hookPath] of [ + ['CJS', CJS_HOOK], + ['Plugin', PLUGIN_HOOK], + ] as const) { + it(`${label}: hook exits silently when all MAX_INFLIGHT slots hold live pids`, async () => { + const { spawn } = await import('child_process'); + const lockDir = path.join(gitNexusDir, '.hook-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + + // Spawn 3 long-sleeping node child processes to use as live PIDs. + const sleepers = [0, 1, 2].map(() => + spawn(process.execPath, ['-e', 'setTimeout(()=>{},60000)'], { + stdio: 'ignore', + detached: false, + }), + ); + const writtenLocks: string[] = []; + try { + for (let i = 0; i < sleepers.length; i++) { + // Slot files are named slot-N.lock; content is the owning PID. + const p = path.join(lockDir, `slot-${i}.lock`); + fs.writeFileSync(p, String(sleepers[i].pid)); + writtenLocks.push(p); + } + + const result = runHook(hookPath, { + hook_event_name: 'PreToolUse', + tool_name: 'Grep', + tool_input: { pattern: 'validateUser' }, + cwd: tmpDir, + }); + + expect(result.stdout.trim()).toBe(''); + // Sentinel slot files survive; the hook bailed before claiming any of them. + for (let i = 0; i < sleepers.length; i++) { + const p = path.join(lockDir, `slot-${i}.lock`); + expect(fs.existsSync(p)).toBe(true); + // Owner unchanged. + expect(fs.readFileSync(p, 'utf-8').trim()).toBe(String(sleepers[i].pid)); + } + } finally { + for (const child of sleepers) { + try { + child.kill(); + } catch { + /* ignore */ + } + } + for (const p of writtenLocks) { + try { + fs.unlinkSync(p); + } catch { + /* ignore */ + } + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); + + it(`${label}: hook reclaims a slot held by a dead pid`, () => { + const lockDir = path.join(gitNexusDir, '.hook-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + // PID 1 exists on every POSIX system (init); on Windows process.kill(1,0) + // throws. Use a definitely-dead PID instead: a very large number unlikely + // to be assigned. + const deadPid = 2_147_483_640; + const stalePath = path.join(lockDir, 'slot-0.lock'); + try { + fs.writeFileSync(stalePath, String(deadPid)); + expect(fs.readFileSync(stalePath, 'utf-8').trim()).toBe(String(deadPid)); + + runHook(hookPath, { + hook_event_name: 'PreToolUse', + tool_name: 'Grep', + tool_input: { pattern: 'validateUser' }, + cwd: tmpDir, + }); + + // The hook reclaimed and then released slot-0 — either the file is + // gone (released) or its content is something other than the dead PID. + if (fs.existsSync(stalePath)) { + expect(fs.readFileSync(stalePath, 'utf-8').trim()).not.toBe(String(deadPid)); + } + } finally { + try { + fs.unlinkSync(stalePath); + } catch { + /* already pruned */ + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); + + it(`${label}: hook does not exceed MAX_INFLIGHT under simultaneous bursts (hard cap)`, async () => { + // Spawn many hook processes concurrently and assert that at most + // MAX_INFLIGHT (3) slot files end up populated by live pids. The + // O_CREAT|O_EXCL slot scheme makes this a hard cap, not the soft cap + // that the count-then-claim approach gives. + const { spawn } = await import('child_process'); + const lockDir = path.join(gitNexusDir, '.hook-locks'); + // Clean any leftover slot files. + try { + for (const f of fs.readdirSync(lockDir)) fs.unlinkSync(path.join(lockDir, f)); + } catch { + /* dir may not exist yet */ + } + fs.mkdirSync(lockDir, { recursive: true }); + + // We use child workers that just claim a slot via the same algorithm + // and then sleep, so we can observe the on-disk state under contention + // without spawning the real gitnexus augment CLI. + const claimerScript = ` + const fs = require('fs'); const path = require('path'); + const lockDir = ${JSON.stringify(lockDir)}; + const MAX = 3; + const STALE = 30000; + const myPid = String(process.pid); + function tryAcquire() { + for (let slot = 0; slot < MAX; slot++) { + const p = path.join(lockDir, 'slot-' + slot + '.lock'); + for (let a = 0; a < 2; a++) { + try { fs.writeFileSync(p, myPid, { flag: 'wx' }); return p; } + catch { + let stat; try { stat = fs.statSync(p); } catch { continue; } + let live = false; + try { + const s = fs.readFileSync(p, 'utf-8').trim(); + if (s === '') live = true; + else { const o = Number.parseInt(s, 10); + if (Number.isFinite(o) && o > 0) { try { process.kill(o, 0); live = true; } catch {} } + } + } catch {} + if (live && Date.now() - stat.mtimeMs > STALE) live = false; + if (live) break; + try { fs.unlinkSync(p); } catch {} + } + } + } + return null; + } + const claimed = tryAcquire(); + if (claimed) { + process.stdout.write('CLAIMED:' + claimed + '\\n'); + setTimeout(() => {}, 5000); + } else { + process.stdout.write('SKIPPED\\n'); + } + `; + + const N = 10; + const claimers = Array.from({ length: N }, () => + spawn(process.execPath, ['-e', claimerScript], { + stdio: ['ignore', 'pipe', 'ignore'], + detached: false, + }), + ); + try { + // Wait until every claimer has printed its decision. + const decisions = await Promise.all( + claimers.map( + (c) => + new Promise((resolve) => { + let buf = ''; + c.stdout!.on('data', (d) => { + buf += d.toString(); + if (buf.includes('\n')) resolve(buf.split('\n')[0]); + }); + c.on('exit', () => resolve(buf.split('\n')[0] || 'EXIT')); + }), + ), + ); + const claimedCount = decisions.filter((d) => d.startsWith('CLAIMED:')).length; + const skippedCount = decisions.filter((d) => d === 'SKIPPED').length; + + // HARD CAP: never more than 3 winners, regardless of how many bursts. + expect(claimedCount).toBeLessThanOrEqual(3); + // And the remainder must have all explicitly skipped. + expect(claimedCount + skippedCount).toBe(N); + + // On-disk state matches. + const liveSlots = fs + .readdirSync(lockDir) + .filter((f) => /^slot-\d+\.lock$/.test(f)) + .filter((f) => { + try { + const o = Number.parseInt(fs.readFileSync(path.join(lockDir, f), 'utf-8').trim(), 10); + return Number.isFinite(o) && o > 0; + } catch { + return false; + } + }); + expect(liveSlots.length).toBeLessThanOrEqual(3); + } finally { + for (const c of claimers) { + try { + c.kill(); + } catch { + /* ignore */ + } + } + try { + for (const f of fs.readdirSync(lockDir)) fs.unlinkSync(path.join(lockDir, f)); + } catch { + /* ignore */ + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); + } +}); + // ─── Integration: PostToolUse staleness detection ─────────────────── describe('PostToolUse staleness detection (integration)', () => {