From 482ce524581346d50da6504bb17c55be9506e9cc Mon Sep 17 00:00:00 2001 From: abhigyanpatwari Date: Tue, 12 May 2026 23:13:44 +0530 Subject: [PATCH] fix(hooks): fail closed when lock dir cannot be created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- gitnexus-claude-plugin/hooks/gitnexus-hook.js | 15 ++++++++---- .../hooks/gitnexus-hook.cjs | 15 ++++++++---- gitnexus/hooks/claude/gitnexus-hook.cjs | 15 ++++++++---- gitnexus/test/unit/cursor-hook.test.ts | 23 +++++++++++++++++++ gitnexus/test/unit/hooks.test.ts | 18 +++++++++++++++ 5 files changed, 71 insertions(+), 15 deletions(-) diff --git a/gitnexus-claude-plugin/hooks/gitnexus-hook.js b/gitnexus-claude-plugin/hooks/gitnexus-hook.js index 577e7f16c..35945df04 100644 --- a/gitnexus-claude-plugin/hooks/gitnexus-hook.js +++ b/gitnexus-claude-plugin/hooks/gitnexus-hook.js @@ -193,8 +193,11 @@ function acquireHookSlot(gitNexusDir) { try { fs.mkdirSync(lockDir, { recursive: true }); } catch { - // Cannot create lock dir — fall through unguarded so the hook still works. - return () => {}; + // 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); @@ -269,9 +272,11 @@ function acquireHookSlot(gitNexusDir) { /* already closed */ } } - // PID-liveness wins over age (avoids evicting a slow-but-alive hook). - // Age check is a safety net against PID reuse on long-abandoned slots: - // 30s >> the 7s augment timeout, so a healthy run never hits it. + // 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; } diff --git a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs index d4c3035fb..010e86157 100644 --- a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs +++ b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs @@ -223,8 +223,11 @@ function acquireHookSlot(gitNexusDir) { try { fs.mkdirSync(lockDir, { recursive: true }); } catch { - // Cannot create lock dir — fall through unguarded so the hook still works. - return () => {}; + // 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); @@ -299,9 +302,11 @@ function acquireHookSlot(gitNexusDir) { /* already closed */ } } - // PID-liveness wins over age (avoids evicting a slow-but-alive hook). - // Age check is a safety net against PID reuse on long-abandoned slots: - // 30s >> the 7s augment timeout, so a healthy run never hits it. + // 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; } diff --git a/gitnexus/hooks/claude/gitnexus-hook.cjs b/gitnexus/hooks/claude/gitnexus-hook.cjs index 23baf9af1..446b758af 100755 --- a/gitnexus/hooks/claude/gitnexus-hook.cjs +++ b/gitnexus/hooks/claude/gitnexus-hook.cjs @@ -193,8 +193,11 @@ function acquireHookSlot(gitNexusDir) { try { fs.mkdirSync(lockDir, { recursive: true }); } catch { - // Cannot create lock dir — fall through unguarded so the hook still works. - return () => {}; + // 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); @@ -269,9 +272,11 @@ function acquireHookSlot(gitNexusDir) { /* already closed */ } } - // PID-liveness wins over age (avoids evicting a slow-but-alive hook). - // Age check is a safety net against PID reuse on long-abandoned slots: - // 30s >> the 7s augment timeout, so a healthy run never hits it. + // 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; } diff --git a/gitnexus/test/unit/cursor-hook.test.ts b/gitnexus/test/unit/cursor-hook.test.ts index 329095c92..9d484adf1 100644 --- a/gitnexus/test/unit/cursor-hook.test.ts +++ b/gitnexus/test/unit/cursor-hook.test.ts @@ -420,6 +420,29 @@ describe('Cursor hook concurrency guard', () => { ); 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 = 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*\}/); + }); + + // 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 ────── diff --git a/gitnexus/test/unit/hooks.test.ts b/gitnexus/test/unit/hooks.test.ts index c34667367..c3ca2a4a5 100644 --- a/gitnexus/test/unit/hooks.test.ts +++ b/gitnexus/test/unit/hooks.test.ts @@ -331,6 +331,24 @@ describe('PreToolUse concurrency guard', () => { ); 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(hookPath, '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*\}/); + }); } });