diff --git a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs index 66b17f84f..29b5b1b6d 100644 --- a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs +++ b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs @@ -340,12 +340,28 @@ function resolveCliPath() { } // The Cursor host enforces hooks.json's postToolUse `timeout` (seconds) -// against the whole hook process. hooks.json ships 60s so a cold -// `npx -y gitnexus` (package download + install) can actually finish; the -// npx fallback budget is then sized from the remaining host budget minus -// headroom for node startup and the final stdout write, instead of a flat -// inner+5s that could overshoot or undercut the deadline. +// against the WHOLE hook process, so the npx fallback can never get more +// than that window however long a cold `npx -y gitnexus` install (package +// download + unpack) actually takes. hooks.json ships 60s for exactly that +// download; this hook then derives the npx budget from what is LEFT of that +// window rather than from the caller's inner-CLI timeout, which was sized +// for an already-installed local CLI and would otherwise pin every cold +// install at ~7s + 5s — the very cost the 60s manifest was raised for. +// +// Three bounds, all applied by resolveNpxTimeoutMs() below: +// HEADROOM — reserved for node startup, the hook-slot release, and the +// final stdout write the host still has to read; +// MIN — below this, the remaining window is too short for a result +// the host could ever observe (the guard's SIGKILL grace, plus +// spawnSync teardown, plus the final write), so the npx +// fallback is SKIPPED outright instead of starting work that +// is guaranteed to be killed mid-flight; +// MAX — upper clamp so one pathologically slow install cannot hold a +// per-repo LadybugDB hook slot for the whole host window and +// starve concurrent edits in the same repo. const CURSOR_NPX_HEADROOM_MS = 5000; +const CURSOR_NPX_MIN_BUDGET_MS = 5000; +const CURSOR_NPX_MAX_BUDGET_MS = 45000; const CURSOR_HOOK_START_MS = Date.now(); const HOOK_ENV = process['env']; @@ -363,41 +379,137 @@ function resolveCursorHostBudgetMs() { return 60000; } -// Resolve a coreutils/BSD `timeout` binary for the npx arm. The CLI behind -// npx is a grandchild (npx -> gitnexus), so the wrap leads with `-s KILL`: -// a plain SIGTERM would kill only the obedient npx parent and let the -// grandchild keep holding the LadybugDB lock after this hook released its -// slot. Mirrors the Claude adapter's wrapped npx arm; Windows stays -// unwrapped (npx is a .cmd script there). +// Decide the npx fallback's budget from the host window that is actually +// left. Returns null when nothing useful fits — the caller must then skip +// the npx spawn entirely rather than floor the budget at some minimum. +// +// Two failure shapes this replaces, both silent: +// - a `Math.max(1000, …)` floor meant an ALREADY-EXPIRED window still +// handed the guarded arm 1000ms (plus a 2000ms spawnSync allowance), so +// the host killed the hook before the guard could return or the final +// stdout write could land. Starting work that cannot finish is strictly +// worse than not starting it; +// - an inner `timeout + 5000` ceiling capped every fallback at the old +// 7s + 5s budget, so the 60s hooks.json manifest could never actually +// pay for a cold `npx -y gitnexus` download. +// +// Deliberately pure and parameterised so the arithmetic is testable without +// spawning npx, and so it does NOT take the caller's inner-CLI `timeout`: +// that value still sizes the direct-exec arm (where it is correct) and is +// meaningless for a cold package download. +function resolveNpxTimeoutMs(hostBudgetMs, elapsedMs) { + const remaining = hostBudgetMs - CURSOR_NPX_HEADROOM_MS - elapsedMs; + if (remaining < CURSOR_NPX_MIN_BUDGET_MS) return null; + return Math.min(remaining, CURSOR_NPX_MAX_BUDGET_MS); +} + +// Sentinel for the module-wide memo below: +// undefined = not resolved yet (resolve lazily, on the first npx fallback) +// string = self-tested coreutils timeout/gtimeout path (use as wrapper) +// null = no usable wrapper (disabled, none found, or self-test failed) +let unixGuardTimeoutCache; + +/** + * A candidate is usable only when it RUNS the wrapped command AND + * PROPAGATES its exit status, proven by + * `timeout -k 1 1 /bin/sh -c 'exit 42'` exiting 42. + * + * This rejects two shapes that mere existence cannot: + * - wrappers WITHOUT coreutils' `-k` flag — busybox <1.34, toybox, broken + * symlinks — which exit with a usage error without ever running npx, so + * the npx grandchild is neither wrapped nor reaped; + * - always-exit-0 stubs (/bin/true shapes), which would be adopted and + * "succeed" every wrapped spawn instantly without running it. Status 0 + * and empty stderr satisfy main()'s `!child.error && child.status === 0` + * check, so the hook would emit NO additional_context at all — a + * silently dead augmentation rather than a visible failure. + */ +function passesGuardSelfTest(guard) { + try { + const selfTest = spawnSync(guard, ['-k', '1', '1', '/bin/sh', '-c', 'exit 42'], { + encoding: 'utf-8', + timeout: 3000, + stdio: ['ignore', 'ignore', 'ignore'], + windowsHide: true, + }); + return !selfTest.error && selfTest.status === 42; + } catch { + return false; + } +} + +/** + * Resolve a coreutils/BSD `timeout` binary for the npx arm. The CLI behind + * npx is a grandchild (npx -> gitnexus), so the wrap leads with `-s KILL`: + * a plain SIGTERM would kill only the obedient npx parent and let the + * grandchild keep holding the LadybugDB lock after this hook released its + * slot. Mirrors the Claude adapter's wrapped npx arm; Windows stays unwrapped + * (npx is a .cmd script there). + * + * GITNEXUS_HOOK_TIMEOUT_PATH semantics (shared with the Claude adapter's + * hook-db-lock-probe.cjs, which owns the canonical contract): the sentinel + * `disabled` turns the wrapper off; any other value is only a CANDIDATE — an + * existing path is tried first, but it must pass the `-k` exit-propagation + * self-test to be adopted. On any failure (non-existent path, directory, + * non-executable file, wrapper without `-k`, always-exit-0 stub) resolution + * falls through to the built-ins below, tried in order, first self-test pass + * wins. Only when EVERY candidate fails does this fall back to the unwrapped + * status quo. This is strictly stronger than merely checking existence: no + * bad env value of ANY shape can silently disable orphan containment. + * + * Memoized per hook process so the resolution — and its one self-test spawn + * — happens at most once however many times runGitNexusCli falls through to + * npx, and so the chosen wrapper stays stable for the process lifetime. (The + * claude adapter memoizes module-wide so probe and adapter share one + * self-test; this integration ships WITHOUT the probe — see main() — so the + * memo exists purely to bound the self-test spawns.) + * + * Residual gap, same as the claude adapter: the self-test proves `-k` exit + * propagation, not process-GROUP signalling. busybox >=1.34 passes it and is + * fully usable for the direct-exec arm, but only signals its direct child, + * so on busybox the `-s KILL` below may not reap the npx -> gitnexus + * grandchild. + */ function resolveUnixGuardTimeout() { - if (process.platform === 'win32') return null; - const override = HOOK_ENV.GITNEXUS_HOOK_TIMEOUT_PATH; - if (override === 'disabled') return null; + if (unixGuardTimeoutCache !== undefined) return unixGuardTimeoutCache; + unixGuardTimeoutCache = null; + if (process.platform === 'win32') return unixGuardTimeoutCache; + const fromEnv = HOOK_ENV.GITNEXUS_HOOK_TIMEOUT_PATH; + const trimmed = fromEnv ? String(fromEnv).trim() : ''; + if (trimmed === 'disabled') return unixGuardTimeoutCache; const candidates = []; - if (override && override.trim()) candidates.push(override.trim()); - candidates.push( + // Resolve the override against THIS process's cwd — the directory the + // existsSync check and the self-test run in — so the cached/returned path + // is always absolute. runGitNexusCli spawns the wrapper with a different + // `cwd` (the tool request's), where a relative value would resolve + // elsewhere (ENOENT) and a slashless name would switch to a PATH lookup. + const override = trimmed ? path.resolve(trimmed) : ''; + if (override && fs.existsSync(override)) candidates.push(override); + for (const builtin of [ '/usr/bin/timeout', - '/usr/local/bin/timeout', - '/opt/homebrew/bin/timeout', - '/usr/bin/gtimeout', - ); - for (const candidate of candidates) { + '/bin/timeout', + // Homebrew's coreutils keg installs the GNU binary as `gtimeout`, NOT + // `timeout` — /opt/homebrew/bin/timeout and /usr/local/bin/timeout do + // not exist on any Homebrew install. Listing the un-prefixed names + // (as an earlier revision did) meant the `-s KILL` orphan-containment + // wrap silently never engaged on exactly the platforms where Homebrew is + // how coreutils gets installed. + '/opt/homebrew/bin/gtimeout', + '/usr/local/bin/gtimeout', + ]) { try { - if (candidate.includes('/') && fs.existsSync(candidate)) return candidate; - const probe = spawnSync('which', [candidate], { - encoding: 'utf-8', - timeout: 3000, - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true, - }); - if (probe.status === 0 && String(probe.stdout).trim()) { - return String(probe.stdout).trim(); - } + if (fs.existsSync(builtin)) candidates.push(builtin); } catch { - /* keep probing */ + /* ignore */ } } - return null; + for (const candidate of candidates) { + if (passesGuardSelfTest(candidate)) { + unixGuardTimeoutCache = candidate; + break; + } + } + return unixGuardTimeoutCache; } function runGitNexusCli(cliPath, args, cwd, timeout) { @@ -412,10 +524,18 @@ function runGitNexusCli(cliPath, args, cwd, timeout) { }); } const elapsed = Date.now() - CURSOR_HOOK_START_MS; - const npxTimeout = Math.max( - 1000, - Math.min(timeout + 5000, resolveCursorHostBudgetMs() - CURSOR_NPX_HEADROOM_MS - elapsed), - ); + const npxTimeout = resolveNpxTimeoutMs(resolveCursorHostBudgetMs(), elapsed); + // Nothing usable is left of the host window, so the npx fallback is SKIPPED + // rather than started. Starting it anyway would only hand the guard a slice + // of an already-expired budget and let the host kill the hook mid-flight — + // which loses the augmentation AND delays the hook-slot release that the + // LadybugDB concurrency guard is holding on our behalf. + // + // Return the same shape spawnSync produces for a child that never ran: + // no `error`, no exit `status`, empty streams. main() already gates on + // `!child.error && child.status === 0`, so this lands on the ordinary + // "no context to emit" path and its `finally` still releases the slot. + if (npxTimeout === null) return { status: null, signal: null, stdout: '', stderr: '' }; const guard = resolveUnixGuardTimeout(); if (guard) { const wrapped = spawnSync( @@ -433,6 +553,9 @@ function runGitNexusCli(cliPath, args, cwd, timeout) { ], { encoding: 'utf-8', + // npxTimeout already excludes CURSOR_NPX_HEADROOM_MS from the host + // window, so this +2000 allowance for spawnSync's own SIGTERM-then- + // wait teardown still lands inside that reserved headroom. timeout: npxTimeout + 2000, cwd, stdio: ['pipe', 'pipe', 'pipe'], @@ -521,4 +644,13 @@ function main() { if (require.main === module) main(); -module.exports = { parseRgGrepPattern, tokenizeShellWords }; +// resolveUnixGuardTimeout / resolveNpxTimeoutMs are exported for the unit +// tests, which exercise the real resolver and budget arithmetic rather than +// grepping this file for substrings. They are pure helpers with no effect on +// the hook's own execution path. +module.exports = { + parseRgGrepPattern, + tokenizeShellWords, + resolveUnixGuardTimeout, + resolveNpxTimeoutMs, +}; diff --git a/gitnexus/test/unit/cursor-hook.test.ts b/gitnexus/test/unit/cursor-hook.test.ts index 88b241a8a..b773320c5 100644 --- a/gitnexus/test/unit/cursor-hook.test.ts +++ b/gitnexus/test/unit/cursor-hook.test.ts @@ -464,8 +464,9 @@ describe('Cursor hook npx fallback host budget', () => { const fnStart = source.indexOf('function runGitNexusCli'); const fnBody = source.slice(fnStart, source.indexOf('\n}\n', fnStart)); - // budget comes from hooks.json (seconds → ms), with headroom applied - expect(fnBody).toContain('resolveCursorHostBudgetMs() - CURSOR_NPX_HEADROOM_MS - elapsed'); + // budget comes from hooks.json (seconds → ms) and the elapsed hook time + // is handed to the pure resolver, which owns the headroom arithmetic + expect(fnBody).toContain('resolveNpxTimeoutMs(resolveCursorHostBudgetMs(), elapsed)'); // the computed budget is what reaches spawnSync (not a bare +5000) expect(fnBody).toContain('timeout: npxTimeout'); expect(fnBody).not.toMatch(/timeout:\s*timeout\s*\+\s*5000/); @@ -482,6 +483,209 @@ describe('Cursor hook npx fallback host budget', () => { }); }); +// ─── Behavioral: npx fallback budget arithmetic ───────────────────── +// resolveNpxTimeoutMs is the whole decision, pure and exported, so these +// assert the real numbers rather than grepping the hook for substrings. + +describe('Cursor hook npx fallback budget', () => { + const hook = require(CURSOR_HOOK) as { + resolveNpxTimeoutMs: (hostBudgetMs: number, elapsedMs: number) => number | null; + }; + const manifest = JSON.parse( + fs.readFileSync(path.join(path.dirname(CURSOR_HOOK), 'hooks.json'), 'utf-8'), + ); + const hostBudgetMs: number = manifest.hooks.postToolUse[0].timeout * 1000; + const source = fs.readFileSync(CURSOR_HOOK, 'utf-8'); + // Same reason as below: the budget comments quote `timeout + 5000` to + // explain what was removed, so strip `//` comments before matching code. + const sourceCode = source + .split('\n') + .map((line) => line.replace(/\/\/.*$/, '')) + .join('\n'); + + it('is no longer hard-capped at the old inner+5s budget', () => { + // main() calls runGitNexusCli(..., 7000), so the previous + // `Math.min(timeout + 5000, ...)` ceiling pinned every npx fallback at + // 12000ms — exactly the cold `npx -y gitnexus` download the 60s manifest + // was raised to accommodate. The budget must now follow the host window. + const fresh = hook.resolveNpxTimeoutMs(hostBudgetMs, 0); + expect(fresh).not.toBeNull(); + expect(fresh as number).toBeGreaterThan(7000 + 5000); + }); + + it('shrinks as the hook burns its host window', () => { + // Past the MAX clamp the budget tracks the remaining window one-for-one: + // each elapsed millisecond is one less millisecond for npx. + const early = hook.resolveNpxTimeoutMs(hostBudgetMs, 20_000) as number; + const later = hook.resolveNpxTimeoutMs(hostBudgetMs, 30_000) as number; + expect(later).toBeLessThan(early); + expect(early - later).toBe(10_000); + }); + + it('returns null once no usable budget remains, instead of a 1s floor', () => { + // `Math.max(1000, ...)` used to hand the guard 1000ms of an ALREADY + // EXPIRED window (plus a 2000ms spawnSync allowance), so the host killed + // the hook before the guard could return or the final stdout write could + // land. Exhausted must mean "do not start npx", never "start it anyway". + expect(hook.resolveNpxTimeoutMs(hostBudgetMs, hostBudgetMs)).toBeNull(); + expect(hook.resolveNpxTimeoutMs(hostBudgetMs, hostBudgetMs + 5_000)).toBeNull(); + // 1ms of window left after CURSOR_NPX_HEADROOM_MS — still unusable + expect(hook.resolveNpxTimeoutMs(hostBudgetMs, hostBudgetMs - 5_000 - 1)).toBeNull(); + }); + + it('still allows a real budget right at the documented floor', () => { + // Floor is CURSOR_NPX_MIN_BUDGET_MS (5000ms of window left after + // CURSOR_NPX_HEADROOM_MS); one millisecond less skips. Pins the boundary + // so the skip cannot drift into rejecting a budget that could actually + // have completed. + const windowLeft = hostBudgetMs - 5_000; // after headroom + expect(hook.resolveNpxTimeoutMs(hostBudgetMs, windowLeft - 5_000)).toBe(5_000); + expect(hook.resolveNpxTimeoutMs(hostBudgetMs, windowLeft - 4_999)).toBeNull(); + }); + + it('clamps to a sane upper bound rather than the whole host window', () => { + // One pathological install must not hold a per-repo hook slot for the + // full 60s window and starve concurrent edits in the same repo. + expect(hook.resolveNpxTimeoutMs(hostBudgetMs, 0)).toBe(45_000); + // ...and the clamp only binds once the remaining window exceeds it + expect(hook.resolveNpxTimeoutMs(hostBudgetMs, 15_000)).toBe(40_000); + }); + + it('no longer mentions the removed inner+5s ceiling anywhere', () => { + expect(sourceCode).not.toMatch(/timeout\s*\+\s*5000/); + }); + + it('skips the npx spawn before resolving a guard when the budget is gone', () => { + // Wiring check (the decision itself is covered behaviourally above): + // the null return has to happen BEFORE the guard self-test spawn and + // before any npx spawnSync, otherwise an exhausted window would still + // pay for a self-test it has no budget to use. + const fnStart = source.indexOf('function runGitNexusCli'); + const fnBody = source.slice(fnStart, source.indexOf('\n}\n', fnStart)); + const skipAt = fnBody.indexOf('if (npxTimeout === null)'); + expect(skipAt).toBeGreaterThan(-1); + expect(fnBody.indexOf('resolveUnixGuardTimeout()')).toBeGreaterThan(skipAt); + expect(fnBody.indexOf('npx.cmd')).toBeGreaterThan(skipAt); + }); +}); + +// ─── Behavioral: guard timeout resolution (F1 + F2) ──────────────── +// resolveUnixGuardTimeout memoizes per module instance, so each case loads +// a FRESH copy of the hook rather than reusing the top-of-file require. + +describe('Cursor hook guard timeout resolution', () => { + const isUnix = process.platform !== 'win32'; + const source = fs.readFileSync(CURSOR_HOOK, 'utf-8'); + + // This hook is deliberately dense with explanatory comments — several of + // them quote the very paths being asserted on (to explain what an earlier + // revision got wrong). Strip `//` comments so these source assertions + // describe CODE, and cannot pass or fail on the prose around it. + function codeOf(fnSignature: string): string { + const start = source.indexOf(fnSignature); + expect(start).toBeGreaterThan(-1); + const body = source.slice(start, source.indexOf('\n}\n', start)); + return body + .split('\n') + .map((line) => line.replace(/\/\/.*$/, '')) + .join('\n'); + } + + function freshHook(): { + resolveUnixGuardTimeout: () => string | null; + } { + delete require.cache[require.resolve(CURSOR_HOOK)]; + return require(CURSOR_HOOK) as { resolveUnixGuardTimeout: () => string | null }; + } + + function withTimeoutPath(value: string | undefined, fn: () => T): T { + const prior = process.env.GITNEXUS_HOOK_TIMEOUT_PATH; + if (value === undefined) delete process.env.GITNEXUS_HOOK_TIMEOUT_PATH; + else process.env.GITNEXUS_HOOK_TIMEOUT_PATH = value; + try { + return fn(); + } finally { + if (prior === undefined) delete process.env.GITNEXUS_HOOK_TIMEOUT_PATH; + else process.env.GITNEXUS_HOOK_TIMEOUT_PATH = prior; + } + } + + it('looks for Homebrew gtimeout, not a Homebrew timeout', () => { + // Homebrew's coreutils keg installs the GNU binary as `gtimeout`. Listing + // `/opt/homebrew/bin/timeout` (as an earlier revision did) meant the + // `-s KILL` orphan-containment wrap silently never engaged on Apple + // Silicon, because every candidate is an absolute path and so the old + // `which` fallback could never discover `gtimeout` either. + const fnBody = codeOf('function resolveUnixGuardTimeout'); + expect(fnBody).toContain('/opt/homebrew/bin/gtimeout'); + expect(fnBody).toContain('/usr/local/bin/gtimeout'); + expect(fnBody).toContain('/usr/bin/timeout'); + expect(fnBody).toContain('/bin/timeout'); + expect(fnBody).not.toContain('/opt/homebrew/bin/timeout'); + expect(fnBody).not.toContain('/usr/local/bin/timeout'); + // The `which` probe is gone: it could never fire for absolute paths and + // would only re-introduce PATH-dependent, unvalidated adoption. + expect(fnBody).not.toContain('which'); + }); + + it.skipIf(!isUnix)('does not adopt an always-exit-0 stub as the guard', () => { + // GITNEXUS_HOOK_TIMEOUT_PATH=/bin/true passes a bare existsSync + // check. The wrapped spawn then "succeeded" instantly without ever + // running npx, and status 0 + empty stderr satisfies main()'s + // `!child.error && child.status === 0` check — so the hook emitted NO + // augmentation at all: a silently dead hook, not a visible failure. + // Hermetic stub (not /bin/true, which macOS 15+ no longer ships). + const stubDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-guard-stub-')); + const stub = path.join(stubDir, 'stub-timeout'); + try { + fs.writeFileSync(stub, '#!/bin/sh\nexit 0\n', { mode: 0o755 }); + const resolved = withTimeoutPath(stub, () => freshHook().resolveUnixGuardTimeout()); + expect(resolved).not.toBe(stub); + } finally { + fs.rmSync(stubDir, { recursive: true, force: true }); + } + }); + + it.skipIf(!isUnix)('resolves a relative override against this process, not the tool cwd', () => { + // runGitNexusCli spawns with the tool request's cwd, so a relative + // override would ENOENT there — or, slashless, silently become a PATH + // lookup. path.resolve makes the adopted path absolute. + const resolved = withTimeoutPath('some/wrapper', () => freshHook().resolveUnixGuardTimeout()); + expect(resolved).not.toBe('some/wrapper'); + }); + + it.skipIf(!isUnix)('honours the disabled sentinel', () => { + expect(withTimeoutPath('disabled', () => freshHook().resolveUnixGuardTimeout())).toBeNull(); + }); + + it.skipIf(!isUnix)( + 'only ever adopts a guard that runs the command AND propagates its exit status', + () => { + // Independent re-derivation of the contract: whatever the resolver + // returns must be a real coreutils timeout/gtimeout AND must actually + // propagate a non-zero exit. null is also a valid answer (no wrapper + // installed here), but never a silently-dead substitute. + const resolved = withTimeoutPath(undefined, () => freshHook().resolveUnixGuardTimeout()); + if (resolved === null) return; // nothing installed to validate + expect(path.basename(resolved)).toMatch(/^g?timeout$/); + const probe = spawnSync(resolved, ['-k', '1', '1', '/bin/sh', '-c', 'exit 42'], { + encoding: 'utf-8', + timeout: 5000, + stdio: ['ignore', 'ignore', 'ignore'], + }); + expect(probe.error).toBeUndefined(); + expect(probe.status).toBe(42); + }, + ); + + it.skipIf(!isUnix)('self-tests candidates with the -k exit-propagation probe', () => { + // The adoption test itself, pinned so it cannot be weakened back to an + // existence check: it must run a wrapped /bin/sh and observe exit 42. + expect(source).toContain("['-k', '1', '1', '/bin/sh', '-c', 'exit 42']"); + expect(source).toContain('!selfTest.error && selfTest.status === 42'); + }); +}); + // ─── Source code regression: concurrency guard (#1486) ───────────── describe('Cursor hook concurrency guard', () => {