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/src/core/search/hybrid-search.ts b/gitnexus/src/core/search/hybrid-search.ts index b76a9f5e9..a2521dd83 100644 --- a/gitnexus/src/core/search/hybrid-search.ts +++ b/gitnexus/src/core/search/hybrid-search.ts @@ -50,9 +50,15 @@ export const mergeWithRRF = ( ): HybridSearchResult[] => { const merged = new Map(); + // Guard against undefined/null inputs (#1489) — when FTS is unavailable + // in the MCP process, bm25Results can arrive as undefined and the + // for-loop would throw "bm25Results is not iterable". + const safeBm25 = bm25Results ?? []; + const safeSemantic = semanticResults ?? []; + // Process BM25 results - for (let i = 0; i < bm25Results.length; i++) { - const r = bm25Results[i]; + for (let i = 0; i < safeBm25.length; i++) { + const r = safeBm25[i]; const rrfScore = 1 / (RRF_K + i + 1); // i+1 because rank starts at 1 merged.set(r.filePath, { @@ -65,8 +71,8 @@ export const mergeWithRRF = ( } // Process semantic results and merge - for (let i = 0; i < semanticResults.length; i++) { - const r = semanticResults[i]; + for (let i = 0; i < safeSemantic.length; i++) { + const r = safeSemantic[i]; const rrfScore = 1 / (RRF_K + i + 1); const existing = merged.get(r.filePath); @@ -149,6 +155,9 @@ export const formatHybridResults = (results: HybridSearchResult[]): string => { * Execute BM25 + semantic search and merge with RRF. * Uses LadybugDB FTS for always-fresh BM25 results (no cached data). * The semanticSearch function is injected to keep this module environment-agnostic. + * + * When FTS is unavailable (e.g. read-only MCP connection, missing indexes), + * falls back to semantic-only results instead of crashing (#1489). */ export const hybridSearch = async ( query: string, @@ -160,8 +169,16 @@ export const hybridSearch = async ( k?: number, ) => Promise, ): Promise => { - // Use LadybugDB FTS for always-fresh BM25 results - const { results: bm25Results } = await searchFTSFromLbug(query, limit); + // Use LadybugDB FTS for always-fresh BM25 results. + // If FTS fails (e.g. extension not loaded in MCP process), fall back to + // semantic-only search instead of crashing with "bm25Results is not iterable". + let bm25Results: BM25SearchResult[] = []; + try { + const ftsResponse = await searchFTSFromLbug(query, limit); + bm25Results = ftsResponse?.results ?? []; + } catch { + // FTS unavailable — continue with semantic-only search + } const semanticResults = await semanticSearch(executeQuery, query, limit); return mergeWithRRF(bm25Results, semanticResults, limit); }; diff --git a/gitnexus/src/core/wiki/generator.ts b/gitnexus/src/core/wiki/generator.ts index 9f65d26e8..dc9c2e74e 100644 --- a/gitnexus/src/core/wiki/generator.ts +++ b/gitnexus/src/core/wiki/generator.ts @@ -28,6 +28,7 @@ import { type FileWithExports, } from './graph-queries.js'; import { generateHTMLViewer } from './html-viewer.js'; +import { sanitizeMermaidMarkdown } from './mermaid-sanitizer.js'; import { callLLM, @@ -591,7 +592,7 @@ export class WikiGenerator { const response = await this.invokeLLM(prompt, MODULE_SYSTEM_PROMPT, this.streamOpts(node.name)); // Write page with front matter - const pageContent = `# ${node.name}\n\n${response.content}`; + const pageContent = sanitizeMermaidMarkdown(`# ${node.name}\n\n${response.content}`); await fs.writeFile(path.join(this.wikiDir, `${node.slug}.md`), pageContent, 'utf-8'); } @@ -631,7 +632,7 @@ export class WikiGenerator { const response = await this.invokeLLM(prompt, PARENT_SYSTEM_PROMPT, this.streamOpts(node.name)); - const pageContent = `# ${node.name}\n\n${response.content}`; + const pageContent = sanitizeMermaidMarkdown(`# ${node.name}\n\n${response.content}`); await fs.writeFile(path.join(this.wikiDir, `${node.slug}.md`), pageContent, 'utf-8'); } @@ -681,7 +682,9 @@ export class WikiGenerator { this.streamOpts('Generating overview', 88), ); - const pageContent = `# ${path.basename(this.repoPath)} — Wiki\n\n${response.content}`; + const pageContent = sanitizeMermaidMarkdown( + `# ${path.basename(this.repoPath)} — Wiki\n\n${response.content}`, + ); await fs.writeFile(path.join(this.wikiDir, 'overview.md'), pageContent, 'utf-8'); } diff --git a/gitnexus/src/core/wiki/html-viewer.ts b/gitnexus/src/core/wiki/html-viewer.ts index f961d36ae..c0fd7323a 100644 --- a/gitnexus/src/core/wiki/html-viewer.ts +++ b/gitnexus/src/core/wiki/html-viewer.ts @@ -7,6 +7,7 @@ import fs from 'fs/promises'; import path from 'path'; +import { sanitizeMermaidMarkdown } from './mermaid-sanitizer.js'; interface ModuleTreeNode { name: string; @@ -42,7 +43,7 @@ export async function generateHTMLViewer(wikiDir: string, projectName: string): const dirEntries = await fs.readdir(wikiDir); for (const f of dirEntries.filter((f) => f.endsWith('.md'))) { const content = await fs.readFile(path.join(wikiDir, f), 'utf-8'); - pages[f.replace(/\.md$/, '')] = content; + pages[f.replace(/\.md$/, '')] = sanitizeMermaidMarkdown(content); } const html = buildHTML(projectName, moduleTree, pages, meta); diff --git a/gitnexus/src/core/wiki/mermaid-sanitizer.ts b/gitnexus/src/core/wiki/mermaid-sanitizer.ts new file mode 100644 index 000000000..c8d443c78 --- /dev/null +++ b/gitnexus/src/core/wiki/mermaid-sanitizer.ts @@ -0,0 +1,119 @@ +const MERMAID_FENCE_RE = /```mermaid\s*\n([\s\S]*?)```/g; +const NODE_LABEL_RE = + /(\[[^\]\n]*(?:\\n)[^\]\n]*\]|\{[^}\n]*(?:\\n)[^}\n]*\}|\([^)\n]*(?:\\n)[^)\n]*\))/g; +const EDGE_LABEL_RE = /\|([^|\n]+)\|/g; +const UNSAFE_EDGE_LABEL_RE = /[()[\]{}<>]/; +const UNSAFE_NODE_ID_RE = /[^A-Za-z0-9_-]/; +const NODE_ID_RE = /^[A-Za-z0-9_.:/()-]+$/; + +const LINE_PREFIX_RE = /^(\s*(?:(?:[-A-Za-z0-9_]+)\s*:\s*)?)(.*)$/; +const EDGE_RE = + /(\s*(?:[ox])?(?:--+|==+|\.\.+)(?:[>|ox])?\|[^|\n]*\|(?:[>|ox])?|\s*(?:[ox])?(?:--+|==+|\.\.+)(?:[>|ox])?|\s*<--+>?\s*)/g; + +export function sanitizeMermaidMarkdown(markdown: string): string { + return markdown.replace(MERMAID_FENCE_RE, (_match, diagram: string) => { + return '```mermaid\n' + sanitizeMermaidDiagram(diagram) + '```'; + }); +} + +export function sanitizeMermaidDiagram(diagram: string): string { + const aliases = new Map(); + let nextAlias = 1; + + const aliasFor = (id: string): string => { + const existing = aliases.get(id); + if (existing) return existing; + + const base = id.replace(/[^A-Za-z0-9_-]/g, '_').replace(/^_+|_+$/g, '') || 'node'; + let alias = base; + while ([...aliases.values()].includes(alias)) { + nextAlias += 1; + alias = `${base}_${nextAlias}`; + } + aliases.set(id, alias); + return alias; + }; + + return diagram + .split('\n') + .map((line) => sanitizeMermaidLine(line, aliasFor)) + .join('\n'); +} + +function sanitizeMermaidLine(line: string, aliasFor: (id: string) => string): string { + let sanitized = replaceLiteralLineBreaksInLabels(line); + sanitized = quoteUnsafeEdgeLabels(sanitized); + + const prefixMatch = sanitized.match(LINE_PREFIX_RE); + if (!prefixMatch) return sanitized; + + const prefix = prefixMatch[1]; + const body = prefixMatch[2]; + if (isDirectiveLine(body)) return sanitized; + + const parts = body.split(EDGE_RE); + if (parts.length === 1) return sanitized; + + for (let i = 0; i < parts.length; i += 2) { + parts[i] = sanitizeNodeReference(parts[i], aliasFor); + } + + return prefix + parts.join(''); +} + +function replaceLiteralLineBreaksInLabels(line: string): string { + return line.replace(NODE_LABEL_RE, (label) => label.replace(/\\n/g, '
')); +} + +function quoteUnsafeEdgeLabels(line: string): string { + return line.replace(EDGE_LABEL_RE, (match, label: string) => { + const trimmed = label.trim(); + if (!UNSAFE_EDGE_LABEL_RE.test(trimmed)) return match; + if ( + (trimmed.startsWith('"') && trimmed.endsWith('"')) || + (trimmed.startsWith("'") && trimmed.endsWith("'")) + ) { + return match; + } + return `|"${escapeMermaidLabel(trimmed)}"|`; + }); +} + +function sanitizeNodeReference(segment: string, aliasFor: (id: string) => string): string { + const match = segment.match(/^(\s*)([A-Za-z0-9_.:/()-]+)(.*?)(\s*)$/); + if (!match) return segment; + + const [, leading, id, suffix, trailing] = match; + if (!NODE_ID_RE.test(id) || !UNSAFE_NODE_ID_RE.test(id)) return segment; + const hasInlineLabel = + suffix.trim().startsWith('[') || suffix.trim().startsWith('(') || suffix.trim().startsWith('{'); + + if (hasInlineLabel) return `${leading}${aliasFor(id)}${suffix}${trailing}`; + + return `${leading}${aliasFor(id)}["${escapeMermaidLabel(id)}"]${suffix}${trailing}`; +} + +function escapeMermaidLabel(label: string): string { + return label.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); +} + +function isDirectiveLine(line: string): boolean { + const trimmed = line.trim(); + return ( + trimmed === '' || + trimmed.startsWith('%%') || + trimmed.startsWith('graph ') || + trimmed.startsWith('flowchart ') || + trimmed.startsWith('sequenceDiagram') || + trimmed.startsWith('classDiagram') || + trimmed.startsWith('stateDiagram') || + trimmed.startsWith('erDiagram') || + trimmed.startsWith('journey') || + trimmed.startsWith('gantt') || + trimmed.startsWith('pie ') || + trimmed.startsWith('mindmap') || + trimmed.startsWith('timeline') || + trimmed.startsWith('subgraph ') || + trimmed === 'end' + ); +} diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 167c1db81..922a69f85 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -755,8 +755,10 @@ export class LocalBackend { timer.time('vector', this.semanticSearch(repo, searchQuery, searchLimit)), ]); - const bm25Results = bm25SearchResult.results; - const ftsUsed = bm25SearchResult.ftsUsed; + // Guard against undefined results (#1489) — when FTS is entirely + // unavailable the search helper may return an unexpected shape. + const bm25Results = bm25SearchResult?.results ?? []; + const ftsUsed = bm25SearchResult?.ftsUsed ?? false; // Merge via reciprocal rank fusion timer.start('merge'); @@ -774,8 +776,9 @@ export class LocalBackend { } } - for (let i = 0; i < semanticResults.length; i++) { - const result = semanticResults[i]; + const safeSemanticResults = semanticResults ?? []; + for (let i = 0; i < safeSemanticResults.length; i++) { + const result = safeSemanticResults[i]; const key = result.nodeId || result.filePath; const rrfScore = 1 / (60 + i); const existing = scoreMap.get(key); @@ -992,7 +995,17 @@ export class LocalBackend { query: string, limit: number, ): Promise<{ results: any[]; ftsUsed: boolean }> { - const { searchFTSFromLbug } = await import('../../core/search/bm25-index.js'); + let searchFTSFromLbug; + try { + ({ searchFTSFromLbug } = await import('../../core/search/bm25-index.js')); + } catch (err: any) { + // Module import can fail in sandboxed MCP contexts (#1489) + logger.warn( + { err: err?.message }, + 'GitNexus: bm25-index.js import failed — falling back to semantic-only', + ); + return { results: [], ftsUsed: false }; + } let ftsResponse; try { ftsResponse = await searchFTSFromLbug(query, limit, repo.id); @@ -1004,8 +1017,10 @@ export class LocalBackend { return { results: [], ftsUsed: false }; } - const bm25Results = ftsResponse.results; - const ftsUsed = ftsResponse.ftsAvailable; + // Guard against unexpected response shape (#1489) — ftsResponse.results + // could be undefined when the FTS extension is unavailable in the MCP process. + const bm25Results = ftsResponse?.results ?? []; + const ftsUsed = ftsResponse?.ftsAvailable ?? false; const results: any[] = []; diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index 8037511bf..99e52c330 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -36,6 +36,7 @@ const FIXTURE_SRC = path.resolve(testDir, '..', 'fixtures', 'mini-repo'); // still works), `afterAll` rms the parent tmpdir. let MINI_REPO: string; let tmpParent: string; +let suiteGitnexusHome: string; // Absolute file:// URL to tsx loader — needed when spawning CLI with cwd // outside the project tree (bare 'tsx' specifier won't resolve there). @@ -49,6 +50,7 @@ beforeAll(() => { // Copy the fixture into an isolated tmpdir named `mini-repo` so that the // `--repo mini-repo` CLI arg (which matches by basename) still works. tmpParent = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-cli-e2e-')); + suiteGitnexusHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-cli-e2e-home-')); MINI_REPO = path.join(tmpParent, 'mini-repo'); fs.cpSync(FIXTURE_SRC, MINI_REPO, { recursive: true }); @@ -75,21 +77,30 @@ afterAll(() => { if (tmpParent) { fs.rmSync(tmpParent, { recursive: true, force: true }); } + if (suiteGitnexusHome) { + fs.rmSync(suiteGitnexusHome, { recursive: true, force: true }); + } }); +function cliEnv(extraEnv: Record = {}) { + return { + ...process.env, + GITNEXUS_HOME: suiteGitnexusHome, + // Pre-set --max-old-space-size so analyzeCommand's ensureHeap() sees it + // and skips the re-exec. The re-exec drops the tsx loader (--import tsx + // is not in process.argv), causing ERR_UNKNOWN_FILE_EXTENSION on .ts files. + NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), + ...extraEnv, + }; +} + function runCli(command: string, cwd: string, timeoutMs = 15000) { return spawnSync(process.execPath, ['--import', tsxImportUrl, cliEntry, command], { cwd, encoding: 'utf8', timeout: timeoutMs, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, - // Pre-set --max-old-space-size so analyzeCommand's ensureHeap() sees it - // and skips the re-exec. The re-exec drops the tsx loader (--import tsx - // is not in process.argv), causing ERR_UNKNOWN_FILE_EXTENSION on .ts files. - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - }, + env: cliEnv(), }); } @@ -103,10 +114,7 @@ function runCliRaw(extraArgs: string[], cwd: string, timeoutMs = 15000) { encoding: 'utf8', timeout: timeoutMs, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - }, + env: cliEnv(), }); } @@ -126,11 +134,7 @@ function runCliWithEnv( encoding: 'utf8', timeout: timeoutMs, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - ...extraEnv, - }, + env: cliEnv(extraEnv), }); } @@ -919,10 +923,7 @@ describe('CLI end-to-end', () => { encoding: 'utf8', timeout: timeoutMs, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - }, + env: cliEnv(), }); } @@ -1042,10 +1043,7 @@ describe('CLI end-to-end', () => { encoding: 'utf8', timeout: 15000, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - }, + env: cliEnv(), }, ); if (result.status === null) return; @@ -1159,10 +1157,7 @@ describe('CLI end-to-end', () => { { cwd: MINI_REPO, stdio: ['ignore', 'pipe', 'pipe'], - env: { - ...process.env, - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - }, + env: cliEnv(), }, ); @@ -1212,10 +1207,7 @@ describe('CLI end-to-end', () => { { cwd: MINI_REPO, stdio: ['ignore', 'pipe', 'pipe'], - env: { - ...process.env, - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - }, + env: cliEnv(), }, ); diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 45e1b71d2..8a9a1a629 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -216,6 +216,19 @@ describe('LocalBackend.callTool', () => { expect(result).not.toHaveProperty('warning'); }); + it('does not crash when searchFTSFromLbug throws (#1489)', async () => { + const { searchFTSFromLbug } = await import('../../src/core/search/bm25-index.js'); + vi.mocked(searchFTSFromLbug).mockRejectedValueOnce(new Error('bm25Results is not iterable')); + (executeParameterized as any).mockResolvedValue([]); + + const result = await backend.callTool('query', { query: 'auth' }); + + // Should still return a valid result shape (semantic-only fallback) + expect(result).toHaveProperty('processes'); + expect(result).toHaveProperty('definitions'); + expect(result).not.toHaveProperty('error'); + }); + it('skips vector index query when VECTOR is unsupported by the platform', async () => { const cap = _captureLogger(); platformMocks.isVectorExtensionSupportedByPlatform.mockReturnValue(false); 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)', () => { diff --git a/gitnexus/test/unit/hybrid-search.test.ts b/gitnexus/test/unit/hybrid-search.test.ts index ee2a4499b..813f78aa7 100644 --- a/gitnexus/test/unit/hybrid-search.test.ts +++ b/gitnexus/test/unit/hybrid-search.test.ts @@ -1,5 +1,5 @@ /** - * P1 Unit Tests: Hybrid Search (mergeWithRRF) + * P1 Unit Tests: Hybrid Search (mergeWithRRF + hybridSearch) * * Tests: mergeWithRRF from hybrid-search.ts * - BM25-only merge @@ -7,12 +7,20 @@ * - Combined ranking * - Limit parameter * - Empty inputs + * - Undefined/null inputs (#1489) + * + * Tests: hybridSearch fallback when FTS unavailable (#1489) */ -import { describe, it, expect } from 'vitest'; -import { mergeWithRRF } from '../../src/core/search/hybrid-search.js'; +import { describe, it, expect, vi } from 'vitest'; +import { mergeWithRRF, hybridSearch } from '../../src/core/search/hybrid-search.js'; import type { BM25SearchResult } from '../../src/core/search/bm25-index.js'; import type { SemanticSearchResult } from '../../src/core/embeddings/types.js'; +vi.mock('../../src/core/search/bm25-index.js', async (importOriginal) => { + const actual = (await importOriginal()) as any; + return { ...actual, searchFTSFromLbug: vi.fn() }; +}); + let bm25Rank = 0; function makeBM25(filePath: string, score: number): BM25SearchResult { return { filePath, score, rank: ++bm25Rank }; @@ -123,4 +131,72 @@ describe('mergeWithRRF', () => { expect(result[0].bm25Score).toBe(15); expect(result[0].semanticScore).toBeCloseTo(0.7); // 1 - distance }); + + // Regression: #1489 — bm25Results is not iterable when FTS unavailable + it('does not crash when bm25Results is undefined (#1489)', () => { + const semantic: SemanticSearchResult[] = [makeSemantic('src/a.ts', 0.1)]; + // Force undefined to simulate the crash path where FTS returns unexpected shape + const result = mergeWithRRF(undefined as any, semantic); + expect(result).toHaveLength(1); + expect(result[0].filePath).toBe('src/a.ts'); + expect(result[0].sources).toEqual(['semantic']); + }); + + it('does not crash when semanticResults is undefined (#1489)', () => { + const bm25: BM25SearchResult[] = [makeBM25('src/a.ts', 10)]; + const result = mergeWithRRF(bm25, undefined as any); + expect(result).toHaveLength(1); + expect(result[0].filePath).toBe('src/a.ts'); + expect(result[0].sources).toEqual(['bm25']); + }); + + it('does not crash when both inputs are undefined (#1489)', () => { + const result = mergeWithRRF(undefined as any, undefined as any); + expect(result).toHaveLength(0); + }); +}); + +// Regression: #1489 — hybridSearch must not crash when FTS is unavailable +describe('hybridSearch — FTS failure fallback (#1489)', () => { + it('falls back to semantic-only when searchFTSFromLbug throws', async () => { + const { searchFTSFromLbug } = await import('../../src/core/search/bm25-index.js'); + vi.mocked(searchFTSFromLbug).mockRejectedValueOnce(new Error('bm25Results is not iterable')); + + const mockExecuteQuery = vi.fn().mockResolvedValue([]); + const mockSemanticSearch = vi + .fn() + .mockResolvedValue([makeSemantic('src/semantic-hit.ts', 0.15)]); + + const results = await hybridSearch('test query', 10, mockExecuteQuery, mockSemanticSearch); + expect(results).toHaveLength(1); + expect(results[0].filePath).toBe('src/semantic-hit.ts'); + expect(results[0].sources).toEqual(['semantic']); + }); + + it('returns empty when both FTS and semantic return nothing', async () => { + const { searchFTSFromLbug } = await import('../../src/core/search/bm25-index.js'); + vi.mocked(searchFTSFromLbug).mockRejectedValueOnce(new Error('FTS unavailable')); + + const mockExecuteQuery = vi.fn().mockResolvedValue([]); + const mockSemanticSearch = vi.fn().mockResolvedValue([]); + + const results = await hybridSearch('test query', 10, mockExecuteQuery, mockSemanticSearch); + expect(results).toHaveLength(0); + }); + + it('works normally when FTS succeeds', async () => { + const { searchFTSFromLbug } = await import('../../src/core/search/bm25-index.js'); + vi.mocked(searchFTSFromLbug).mockResolvedValueOnce({ + results: [{ filePath: 'src/fts-hit.ts', score: 5, rank: 1 }], + ftsAvailable: true, + }); + + const mockExecuteQuery = vi.fn().mockResolvedValue([]); + const mockSemanticSearch = vi.fn().mockResolvedValue([]); + + const results = await hybridSearch('test query', 10, mockExecuteQuery, mockSemanticSearch); + expect(results).toHaveLength(1); + expect(results[0].filePath).toBe('src/fts-hit.ts'); + expect(results[0].sources).toEqual(['bm25']); + }); }); diff --git a/gitnexus/test/unit/wiki-mermaid-sanitizer.test.ts b/gitnexus/test/unit/wiki-mermaid-sanitizer.test.ts new file mode 100644 index 000000000..e8c74c8cb --- /dev/null +++ b/gitnexus/test/unit/wiki-mermaid-sanitizer.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from 'vitest'; + +import { + sanitizeMermaidDiagram, + sanitizeMermaidMarkdown, +} from '../../src/core/wiki/mermaid-sanitizer.js'; + +describe('sanitizeMermaidMarkdown', () => { + it('replaces literal newline escapes inside rectangle and diamond labels', () => { + const markdown = [ + '```mermaid', + 'flowchart TD', + ' A[HTTP request\\nwith ID param] --> B{Preceding tei:zone\\nwith @start=#pid?}', + '```', + ].join('\n'); + + const sanitized = sanitizeMermaidMarkdown(markdown); + + expect(sanitized).toContain('A[HTTP request
with ID param]'); + expect(sanitized).toContain('B{Preceding tei:zone
with @start=#pid?}'); + expect(sanitized).not.toContain('\\n'); + }); + + it('quotes unsafe edge labels without changing safe labels', () => { + const diagram = [ + 'graph LR', + ' Script -->|doc()| eXist[(eXist-db XML)]', + ' Client -->|HTTP params| Script', + ].join('\n'); + + const sanitized = sanitizeMermaidDiagram(diagram); + + expect(sanitized).toContain('Script -->|"doc()"| eXist[(eXist-db XML)]'); + expect(sanitized).toContain('Client -->|HTTP params| Script'); + }); + + it('escapes backslashes and quotes in quoted edge labels', () => { + const diagram = ['graph LR', ' Script -->|doc("C:\\\\tmp")| Target'].join('\n'); + + const sanitized = sanitizeMermaidDiagram(diagram); + + expect(sanitized).toContain('Script -->|"doc(\\"C:\\\\\\\\tmp\\")"| Target'); + }); + + it('aliases bare node IDs that contain dots and keeps display labels', () => { + const diagram = [ + 'graph LR', + ' Client -->|xmlurl + xslurl| xslt-conversion.xq', + ' xslt-conversion.xq -->|stream-transform| lbpwebjs-main.xsl', + ' lbpwebjs-main.xsl -->|fetches| TEI-XML[(TEI XML in eXist)]', + ].join('\n'); + + const sanitized = sanitizeMermaidDiagram(diagram); + + expect(sanitized).toContain( + 'Client -->|xmlurl + xslurl| xslt-conversion_xq["xslt-conversion.xq"]', + ); + expect(sanitized).toContain( + 'xslt-conversion_xq["xslt-conversion.xq"] -->|stream-transform| lbpwebjs-main_xsl["lbpwebjs-main.xsl"]', + ); + expect(sanitized).toContain( + 'lbpwebjs-main_xsl["lbpwebjs-main.xsl"] -->|fetches| TEI-XML[(TEI XML in eXist)]', + ); + }); + + it('aliases unsafe node IDs while preserving existing inline labels', () => { + const diagram = [ + 'graph LR', + ' file.name.ts[(eXist-db XML)] --> target.node["Target node"]', + ].join('\n'); + + const sanitized = sanitizeMermaidDiagram(diagram); + + expect(sanitized).toContain('file_name_ts[(eXist-db XML)] --> target_node["Target node"]'); + }); + + it('only rewrites fenced Mermaid blocks in markdown', () => { + const markdown = [ + 'Regular text with doc() and file.name.ts.', + '', + '```ts', + 'const label = "A\\nB";', + '```', + '', + '```mermaid', + 'flowchart LR', + ' A -->|doc()| file.name.ts', + '```', + ].join('\n'); + + const sanitized = sanitizeMermaidMarkdown(markdown); + + expect(sanitized).toContain('Regular text with doc() and file.name.ts.'); + expect(sanitized).toContain('const label = "A\\nB";'); + expect(sanitized).toContain('A -->|"doc()"| file_name_ts["file.name.ts"]'); + }); +});