From 1408bfbffe69422e7db3f004d1cbc556b1634c41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Wed, 8 Jul 2026 18:34:05 +0100 Subject: [PATCH] fix(hook): emit MCP query hint when server owns DB lock (#2396) (#2397) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hook): emit MCP query hint when server owns DB lock (#2396) When the GitNexus MCP server holds the lbug write lock, the PreToolUse hook's CLI `augment` cannot run (LadybugDB is single-writer) and previously skipped silently — disabling graph augmentation in the most common deployment (server online). Since the same session already has the MCP `query` tool live, the owner branch now emits an additionalContext hint pointing the agent at mcp__gitnexus__query for that pattern, via the same sanctioned stdout channel the augment-success path uses (Codex-safe, #2369). Rejected the alternative of having the hook query the server: it runs over stdio (no port/pipe from the separate hook process) and cross-process read-only access can't coexist with the write lock — both are large architecture changes. Applied to all three gated hook copies (claude .cjs, claude-plugin .js, antigravity .cjs); the cursor hook has no owner gate and is untouched. The stderr `augment skipped: MCP server owns DB` diagnostic stays GITNEXUS_DEBUG-gated (#1913). Owner-path tests flipped from stdout-empty to hint-present. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(hook): reword MCP-query hint to be conditionally truthful (#2396) The #2396 owner branch emits the hint on every DB-owner path — a confirmed `gitnexus mcp` owner, a `gitnexus serve` owner, and the fail-closed/timeout paths (the probe collapses timeout and owned to one boolean). The old text claimed "Knowledge graph is live via the MCP server" and named mcp__gitnexus__query unconditionally, which is untrue on a fail-closed probe where no server is confirmed and misdirecting for a serve-only owner (review C2/C4). Reword the hint (byte-identical across all three hook copies) to state that local augment is unavailable and to condition the MCP call on the tools actually being live ("if the GitNexus MCP tools are live in this session"). This is truthful on every owner path; the needles the assertions rely on (mcp__gitnexus__query, query, search_query, the pattern) are preserved. Fix the 10 stale owner/fail-closed unit tests that still asserted empty stdout (review C1, the macOS platform-sensitive 2/3 blocker): flip them to assert the hint via parseHookOutput, keep their stderr/GITNEXUS_DEBUG expectations, and rename the two 'SILENTLY' titles. The GITNEXUS_DEBUG='' owner-hint case is restored (the PR's new loop only covered '0'/'false'). Probe and its white-box tests untouched. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(hook): de-orphan the JSDoc in the claude hook copy (#2396) The #2396 change inserted buildMcpQueryHint between the pre-existing "PreToolUse handler" JSDoc and handlePreToolUse, orphaning that doc onto the helper and leaving handlePreToolUse undocumented (review C5). Move the helper (with its own doc) above the handler doc so the "PreToolUse handler" comment again precedes handlePreToolUse, matching the clean plugin copy. Pure move; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(hook): throttle the MCP-owner hint to once per repo per window (#2396) Previously the hint emitted on every qualifying search while a GitNexus process owned the DB, so an owner-locked session (the common deploy) was nudged toward the MCP query tool on every Grep/Glob/Bash — context bloat and ~2x query amplification (review C3). Add shouldEmitMcpHint(gitNexusDir) to all three hook copies: a per-repo .gitnexus/.mcp-hint-shown mtime marker emits the hint at most once per window. Window via GITNEXUS_MCP_HINT_THROTTLE_MS (default 10min; 0/invalid disables). Best-effort — any fs error falls back to emitting, so the hint is never lost to a marker failure. The stderr skip diagnostic still fires regardless (only the hint is throttled). Tests: hookEnv disables the throttle by default (gitNexusDir is shared across the suite, so a marker would otherwise throttle sibling owner tests); a dedicated macOS-lane test sets a real window and asserts emit-then-throttle with the marker gating it. Co-Authored-By: Claude Opus 4.8 (1M context) * docs(hook): README reflects the MCP-owner query hint, not a silent skip (#2396) The 'Hook augmentation/notifications are silently skipped' section still described the MCP-server-owns-DB path as a silent augmentation skip (review docs finding). That path now hands the agent a conditional MCP-query hint via additionalContext (throttled per repo). Reword the section to describe the hint and its GITNEXUS_MCP_HINT_THROTTLE_MS throttle, and keep the GITNEXUS_DEBUG stderr-diagnostic guidance. No CHANGELOG edit (owned at release time). Co-Authored-By: Claude Opus 4.8 (1M context) * test(hook): guard hint-copy drift + pattern JSON-escaping (#2396) Two gaps the review flagged (R7): - Drift guard: buildMcpQueryHint and shouldEmitMcpHint are triplicated across the three hook copies with no shared module. A source-level byte-identity check (runs on every platform, unlike the macOS-only owner tests) fails if any copy diverges — the institutional pattern the repo already uses for mirrored hook metadata. - Escaping: an adversarial Grep pattern (embedded quote + newline) must not break the additionalContext JSON envelope. A macOS-lane owner test drives the real hook with such a pattern and asserts parseHookOutput still yields valid JSON containing the literal characters (JSON.stringify escapes them). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- gitnexus-claude-plugin/hooks/gitnexus-hook.js | 66 ++++- gitnexus/README.md | 25 +- .../antigravity/gitnexus-antigravity-hook.cjs | 53 +++- gitnexus/hooks/claude/gitnexus-hook.cjs | 68 ++++- .../integration/antigravity-hook-e2e.test.ts | 41 +-- gitnexus/test/unit/hooks.test.ts | 244 +++++++++++++++--- gitnexus/test/utils/hook-test-helpers.ts | 4 + 7 files changed, 424 insertions(+), 77 deletions(-) diff --git a/gitnexus-claude-plugin/hooks/gitnexus-hook.js b/gitnexus-claude-plugin/hooks/gitnexus-hook.js index 021dce60c..a53d79f29 100644 --- a/gitnexus-claude-plugin/hooks/gitnexus-hook.js +++ b/gitnexus-claude-plugin/hooks/gitnexus-hook.js @@ -381,6 +381,49 @@ function sendHookResponse(hookEventName, message) { ); } +/** + * Fallback augmentation for the #2396 path: when a GitNexus process holds the + * lbug DB write lock the CLI `augment` can't run, so point the agent at the MCP + * `query` tool instead. Phrased conditionally ("if the MCP tools are live") so it + * stays truthful on every owner path — a confirmed MCP owner, a `serve` owner, or + * a fail-closed probe where no server is actually confirmed. `pattern` is embedded + * verbatim; the caller (sendHookResponse) JSON-escapes it structurally. + */ +function buildMcpQueryHint(pattern) { + return ( + `[GitNexus] Local augment is unavailable (the graph DB is held by another ` + + `GitNexus process). If the GitNexus MCP tools are live in this session, call ` + + `the GitNexus \`query\` MCP tool (e.g. mcp__gitnexus__query) with ` + + `search_query "${pattern}".` + ); +} + +/** + * #2396 throttle: emit the MCP-query hint at most once per repo per window, so an + * owner-locked session isn't nudged on every search. Window (ms) via + * GITNEXUS_MCP_HINT_THROTTLE_MS (default 10min; 0/invalid disables). Best-effort — + * any fs error falls back to emitting. + * ponytail: per-repo mtime marker, shared across concurrent sessions on the same + * repo; add per-session dedup only if that sharing becomes a problem. + */ +function shouldEmitMcpHint(gitNexusDir) { + const raw = process.env.GITNEXUS_MCP_HINT_THROTTLE_MS; + const windowMs = raw === undefined || raw === '' ? 600000 : Number(raw); + if (!Number.isFinite(windowMs) || windowMs <= 0) return true; + const marker = path.join(gitNexusDir, '.mcp-hint-shown'); + try { + if (Date.now() - fs.statSync(marker).mtimeMs < windowMs) return false; + } catch { + /* marker missing/unreadable → emit */ + } + try { + fs.writeFileSync(marker, ''); + } catch { + /* best-effort; still emit */ + } + return true; +} + /** * PreToolUse handler — augment searches with graph context. */ @@ -417,17 +460,24 @@ function handlePreToolUse(input) { let result = ''; try { if (hasGitNexusServerOwner(gitNexusDir)) { - // Normal skip path: the MCP server owns the DB, so the CLI augment would - // contend on the lock. Stay silent for strict hook runners (issue #1913); - // surface the reason only when diagnostics are explicitly requested. + // #2396: the MCP server holds the DB write lock, so a competing CLI + // `augment` would only contend on it (LadybugDB is single-writer). But the + // session that triggered this hook has the GitNexus MCP tools live — route + // the augmentation to the agent via additionalContext instead of silently + // doing nothing. Mirror the skip reason to stderr only under GITNEXUS_DEBUG + // (strict-runner contract, #1913); the hint itself rides the sanctioned + // additionalContext stdout channel the successful augment already uses. if (isDebugEnabled()) { process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); } - return; - } - const child = runGitNexusCli(['augment', '--', pattern], cwd, 7000); - if (!child.error && child.status === 0) { - result = extractAugmentContext(child.stderr || ''); + if (shouldEmitMcpHint(gitNexusDir)) { + result = buildMcpQueryHint(pattern); + } + } else { + const child = runGitNexusCli(['augment', '--', pattern], cwd, 7000); + if (!child.error && child.status === 0) { + result = extractAugmentContext(child.stderr || ''); + } } } catch { /* graceful failure */ diff --git a/gitnexus/README.md b/gitnexus/README.md index 67e9d39a8..9f88a954e 100644 --- a/gitnexus/README.md +++ b/gitnexus/README.md @@ -535,17 +535,26 @@ After scope resolution, analyze prunes inert block-local value symbols (a functi Programmatic callers can pass `keepLocalValueSymbols: true` in `PipelineOptions` instead of setting the env var. -### Hook augmentation/notifications are silently skipped +### Hook augmentation and skip diagnostics -The Claude Code / Antigravity hooks intentionally stay **silent** on normal skip +The Claude Code / Antigravity hooks keep their **stderr** silent on normal skip paths so strict hook runners (e.g. Codex `PreToolUse`) never see unexpected -output. A search may not be augmented — or a stale-index reminder may not appear -on stderr — when the GitNexus MCP server owns the repo DB, when the DB-lock probe -times out and fails closed, or when the index is already current. +diagnostic output. -To see why a hook skipped, set `GITNEXUS_DEBUG=1` and re-run the action — the hook -writes the reason (e.g. `[GitNexus] augment skipped: MCP server owns DB`) and the -stale-index hint to its stderr: +When a GitNexus process holds the repo DB write lock (the common case — the MCP +server is running, or the DB-lock probe timed out and failed closed), the local +CLI `augment` can't run (LadybugDB is single-writer). Rather than drop the +augmentation, the hook hands the agent a short, conditional MCP-query hint on +stdout (the sanctioned `additionalContext` channel) — _"if the GitNexus MCP tools +are live in this session, call `query` …"_ — so an agent that has the tools can +still fetch graph-ranked context. The hint is throttled to at most once per repo +per window (`GITNEXUS_MCP_HINT_THROTTLE_MS`, default 10 min; `0` disables), so an +owner-locked session isn't nudged on every search. A stale-index reminder, or an +already-current index, stays silent. + +To see why a hook skipped the CLI augment, set `GITNEXUS_DEBUG=1` and re-run the +action — the hook writes the reason (e.g. `[GitNexus] augment skipped: MCP server +owns DB`) and the stale-index hint to its stderr: ```bash GITNEXUS_DEBUG=1 # surfaces hook skip/diagnostic reasons on stderr diff --git a/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs b/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs index 6b9e8b9f8..3331ae3fc 100755 --- a/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs +++ b/gitnexus/hooks/antigravity/gitnexus-antigravity-hook.cjs @@ -391,6 +391,49 @@ function buildAfterToolContext(input) { return parts.length > 0 ? parts.join('\n\n') : null; } +/** + * Fallback augmentation for the #2396 path: when a GitNexus process holds the + * lbug DB write lock the CLI `augment` can't run, so point the agent at the MCP + * `query` tool instead. Phrased conditionally ("if the MCP tools are live") so it + * stays truthful on every owner path — a confirmed MCP owner, a `serve` owner, or + * a fail-closed probe where no server is actually confirmed. `pattern` is embedded + * verbatim; the caller (writeAdditionalContext) JSON-escapes it structurally. + */ +function buildMcpQueryHint(pattern) { + return ( + `[GitNexus] Local augment is unavailable (the graph DB is held by another ` + + `GitNexus process). If the GitNexus MCP tools are live in this session, call ` + + `the GitNexus \`query\` MCP tool (e.g. mcp__gitnexus__query) with ` + + `search_query "${pattern}".` + ); +} + +/** + * #2396 throttle: emit the MCP-query hint at most once per repo per window, so an + * owner-locked session isn't nudged on every search. Window (ms) via + * GITNEXUS_MCP_HINT_THROTTLE_MS (default 10min; 0/invalid disables). Best-effort — + * any fs error falls back to emitting. + * ponytail: per-repo mtime marker, shared across concurrent sessions on the same + * repo; add per-session dedup only if that sharing becomes a problem. + */ +function shouldEmitMcpHint(gitNexusDir) { + const raw = process.env.GITNEXUS_MCP_HINT_THROTTLE_MS; + const windowMs = raw === undefined || raw === '' ? 600000 : Number(raw); + if (!Number.isFinite(windowMs) || windowMs <= 0) return true; + const marker = path.join(gitNexusDir, '.mcp-hint-shown'); + try { + if (Date.now() - fs.statSync(marker).mtimeMs < windowMs) return false; + } catch { + /* marker missing/unreadable → emit */ + } + try { + fs.writeFileSync(marker, ''); + } catch { + /* best-effort; still emit */ + } + return true; +} + function runAugment(gitNexusDir, cwd, pattern) { // Acquire the per-repo slot BEFORE the DB-owner probe (#2163): the probe // itself spawns lsof/ps, so it must be bounded by the same ≤3-per-repo cap @@ -410,12 +453,16 @@ function runAugment(gitNexusDir, cwd, pattern) { } try { if (hasGitNexusServerOwner(gitNexusDir)) { - // Normal skip path: the MCP server owns the DB. Stay silent for strict - // hook runners (issue #1913); surface the reason only under GITNEXUS_DEBUG. + // #2396: the MCP server holds the DB write lock, so a competing CLI + // `augment` would only contend on it (LadybugDB is single-writer). The + // session has the GitNexus MCP tools live — route the augmentation to the + // agent via additionalContext instead of dropping it. Mirror the skip + // reason to stderr only under GITNEXUS_DEBUG (strict-runner contract, + // #1913); the hint itself rides the sanctioned additionalContext channel. if (isDebugEnabled()) { process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); } - return ''; + return shouldEmitMcpHint(gitNexusDir) ? buildMcpQueryHint(pattern) : ''; } const cliPath = resolveCliPath(); const child = runGitNexusCli(cliPath, ['augment', '--', pattern], cwd, 7000); diff --git a/gitnexus/hooks/claude/gitnexus-hook.cjs b/gitnexus/hooks/claude/gitnexus-hook.cjs index 2f24a6f4d..18be614f4 100755 --- a/gitnexus/hooks/claude/gitnexus-hook.cjs +++ b/gitnexus/hooks/claude/gitnexus-hook.cjs @@ -349,6 +349,49 @@ function runGitNexusCli(cliPath, args, cwd, timeout) { }); } +/** + * Fallback augmentation for the #2396 path: when a GitNexus process holds the + * lbug DB write lock the CLI `augment` can't run, so point the agent at the MCP + * `query` tool instead. Phrased conditionally ("if the MCP tools are live") so it + * stays truthful on every owner path — a confirmed MCP owner, a `serve` owner, or + * a fail-closed probe where no server is actually confirmed. `pattern` is embedded + * verbatim; the caller (sendHookResponse) JSON-escapes it structurally. + */ +function buildMcpQueryHint(pattern) { + return ( + `[GitNexus] Local augment is unavailable (the graph DB is held by another ` + + `GitNexus process). If the GitNexus MCP tools are live in this session, call ` + + `the GitNexus \`query\` MCP tool (e.g. mcp__gitnexus__query) with ` + + `search_query "${pattern}".` + ); +} + +/** + * #2396 throttle: emit the MCP-query hint at most once per repo per window, so an + * owner-locked session isn't nudged on every search. Window (ms) via + * GITNEXUS_MCP_HINT_THROTTLE_MS (default 10min; 0/invalid disables). Best-effort — + * any fs error falls back to emitting. + * ponytail: per-repo mtime marker, shared across concurrent sessions on the same + * repo; add per-session dedup only if that sharing becomes a problem. + */ +function shouldEmitMcpHint(gitNexusDir) { + const raw = process.env.GITNEXUS_MCP_HINT_THROTTLE_MS; + const windowMs = raw === undefined || raw === '' ? 600000 : Number(raw); + if (!Number.isFinite(windowMs) || windowMs <= 0) return true; + const marker = path.join(gitNexusDir, '.mcp-hint-shown'); + try { + if (Date.now() - fs.statSync(marker).mtimeMs < windowMs) return false; + } catch { + /* marker missing/unreadable → emit */ + } + try { + fs.writeFileSync(marker, ''); + } catch { + /* best-effort; still emit */ + } + return true; +} + /** * PreToolUse handler — augment searches with graph context. */ @@ -385,18 +428,25 @@ function handlePreToolUse(input) { let result = ''; try { if (hasGitNexusServerOwner(gitNexusDir)) { - // Normal skip path: the MCP server owns the DB, so the CLI augment would - // contend on the lock. Stay silent for strict hook runners (issue #1913); - // surface the reason only when diagnostics are explicitly requested. + // #2396: the MCP server holds the DB write lock, so a competing CLI + // `augment` would only contend on it (LadybugDB is single-writer). But the + // session that triggered this hook has the GitNexus MCP tools live — route + // the augmentation to the agent via additionalContext instead of silently + // doing nothing. Mirror the skip reason to stderr only under GITNEXUS_DEBUG + // (strict-runner contract, #1913); the hint itself rides the sanctioned + // additionalContext stdout channel the successful augment already uses. if (isDebugEnabled()) { process.stderr.write('[GitNexus] augment skipped: MCP server owns DB\n'); } - return; - } - const cliPath = resolveCliPath(); - const child = runGitNexusCli(cliPath, ['augment', '--', pattern], cwd, 7000); - if (!child.error && child.status === 0) { - result = extractAugmentContext(child.stderr || ''); + if (shouldEmitMcpHint(gitNexusDir)) { + result = buildMcpQueryHint(pattern); + } + } else { + const cliPath = resolveCliPath(); + const child = runGitNexusCli(cliPath, ['augment', '--', pattern], cwd, 7000); + if (!child.error && child.status === 0) { + result = extractAugmentContext(child.stderr || ''); + } } } catch { /* graceful failure */ diff --git a/gitnexus/test/integration/antigravity-hook-e2e.test.ts b/gitnexus/test/integration/antigravity-hook-e2e.test.ts index b99005a91..98e64fbce 100644 --- a/gitnexus/test/integration/antigravity-hook-e2e.test.ts +++ b/gitnexus/test/integration/antigravity-hook-e2e.test.ts @@ -416,14 +416,16 @@ describe('antigravity hook adapter e2e', () => { }); }); - // Issue #1913: when a GitNexus MCP server owns the repo DB, runAugment() must - // SKIP — silently by default so strict hook runners never see unexpected - // output, and surface the reason only under GITNEXUS_DEBUG=1. The Claude/Plugin - // copies are covered in test/unit/hooks.test.ts; the antigravity adapter shares - // the identical gated skip and is exercised here through the install pipeline - // (its lock/probe helpers only resolve from the install dir). A faked lsof/ps + - // an empty `lbug` lock force hasGitNexusServerOwner() => true; a marker-writing - // fake CLI proves augment never ran. + // #2396: when a GitNexus MCP server owns the repo DB, runAugment() cannot run + // the CLI augment (LadybugDB is single-writer), so it returns an MCP-query hint + // that reaches the agent via additionalContext instead of dropping the + // augmentation. #1913: the stderr skip diagnostic stays gated behind + // GITNEXUS_DEBUG=1. The Claude/Plugin copies are covered in + // test/unit/hooks.test.ts; the antigravity adapter shares the identical path + // and is exercised here through the install pipeline (its lock/probe helpers + // only resolve from the install dir). A faked lsof/ps + an empty `lbug` lock + // force hasGitNexusServerOwner() => true; a marker-writing fake CLI proves the + // CLI augment never ran. // // #2180: skipped on Linux too — the probe's Linux backend no longer uses // lsof/ps, so the faked lsof/ps can't force owner=true there. This stays as the @@ -431,14 +433,14 @@ describe('antigravity hook adapter e2e', () => { // gated owner-skip with the claude/plugin copies, whose Linux owner detection // is covered against a fake /proc in test/unit/hook-db-lock-probe.test.ts. describe.skipIf(process.platform === 'win32' || process.platform === 'linux')( - 'AfterTool — augment skipped when MCP server owns the DB (#1913)', + 'AfterTool — MCP-query hint when MCP server owns the DB (#2396)', () => { const OWNER_PROBE = { lsofOutput: '12345\n', psOutput: 'node /tmp/node_modules/.bin/gitnexus mcp\n', }; - it('stays SILENT by default (no augment ran, no stderr noise, exit 0)', () => { + it('emits the MCP-query hint on stdout, no stderr noise, exit 0 (CLI augment never ran)', () => { const markerPath = path.join(os.tmpdir(), `antigravity-skip-silent-${process.pid}`); const lbugPath = path.join(gitNexusDir, 'lbug'); fs.writeFileSync(lbugPath, ''); @@ -459,13 +461,15 @@ describe('antigravity hook adapter e2e', () => { ); expect(result.status).toBe(0); - // Strict-runner contract: completely silent — empty stdout AND stderr - // (matches the unit suite's assertion strength for the claude/plugin copies). - expect(result.stdout.trim()).toBe(''); + // #2396: the augmentation is handed to the agent as an MCP-query hint on + // stdout; stderr stays silent (strict-runner contract, #1913). + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); + expect(output!.additionalContext).toContain('validateUser'); expect(result.stderr.trim()).toBe(''); - // Marker absent ⇒ the CLI never ran (augment short-circuited at the owner - // check). The paired GITNEXUS_DEBUG=1 test below positively proves the skip - // was the owner path (it asserts the owner-skip diagnostic on stderr). + // Marker absent ⇒ the CLI never ran (short-circuited at the owner check). + // The paired GITNEXUS_DEBUG=1 test below positively proves the path was + // the owner path (it asserts the owner-skip diagnostic on stderr). expect(fs.existsSync(markerPath)).toBe(false); } finally { fs.rmSync(lbugPath, { force: true }); @@ -495,7 +499,10 @@ describe('antigravity hook adapter e2e', () => { ); expect(result.status).toBe(0); - expect(parseHookOutput(result.stdout)).toBeNull(); + // The hint still rides stdout; GITNEXUS_DEBUG only adds the stderr reason. + expect(parseHookOutput(result.stdout)!.additionalContext).toContain( + 'mcp__gitnexus__query', + ); expect(result.stderr).toContain('[GitNexus] augment skipped: MCP server owns DB'); expect(fs.existsSync(markerPath)).toBe(false); } finally { diff --git a/gitnexus/test/unit/hooks.test.ts b/gitnexus/test/unit/hooks.test.ts index afc4c2f3d..82eeb19f9 100644 --- a/gitnexus/test/unit/hooks.test.ts +++ b/gitnexus/test/unit/hooks.test.ts @@ -1817,13 +1817,14 @@ describe('PreToolUse augmentation filtering (integration)', () => { } }); - // Issue #1913: the MCP-owned-DB skip is a NORMAL (non-error) path, so by - // default it must stay completely silent — empty stdout AND empty stderr, - // exit 0 — so strict hook runners (e.g. Codex `PreToolUse`) never see - // unexpected output. GITNEXUS_DEBUG is forced off to keep the assertion - // deterministic regardless of the ambient environment. + // #2396: when a GitNexus MCP process owns the repo DB the CLI augment can't + // run, so the hook hands the agent the MCP-query hint on stdout (the sanctioned + // additionalContext channel). By default (GITNEXUS_DEBUG unset) the stderr skip + // diagnostic stays silent, so strict hook runners (e.g. Codex `PreToolUse`) see + // no unexpected diagnostic noise — only the augmentation itself (#1913). This is + // the GITNEXUS_DEBUG='' owner-hint coverage; the debug variants are below. it.skipIf(SKIP_LSOF_PATH)( - `${label}: skips augment SILENTLY when a GitNexus MCP process owns the repo DB`, + `${label}: emits the MCP-query hint on stdout, stderr silent by default, when a GitNexus MCP process owns the repo DB`, () => { const markerPath = path.join(os.tmpdir(), `gitnexus-hook-called-${process.pid}-${label}`); const lbugPath = path.join(gitNexusDir, 'lbug'); @@ -1847,7 +1848,9 @@ describe('PreToolUse augmentation filtering (integration)', () => { { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '' } }, ); - expect(result.stdout.trim()).toBe(''); + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); + expect(output!.additionalContext).toContain('validateUser'); expect(result.stderr.trim()).toBe(''); expect(result.status).toBe(0); expect(fs.existsSync(markerPath)).toBe(false); @@ -1859,11 +1862,13 @@ describe('PreToolUse augmentation filtering (integration)', () => { }, ); - // Issue #1913: the skip reason remains recoverable for operators who opt in - // via GITNEXUS_DEBUG=1 — stdout stays empty (no augment ran), the diagnostic - // appears on stderr. + // #2396: when the MCP server owns the DB the CLI augment can't run, so the + // hook hands the agent an MCP-query hint on stdout (the sanctioned + // additionalContext channel) instead of doing nothing. The CLI still never + // spawns (marker absent). #1913: the stderr skip diagnostic stays gated + // behind GITNEXUS_DEBUG. it.skipIf(SKIP_LSOF_PATH)( - `${label}: surfaces the MCP-owner skip reason only under GITNEXUS_DEBUG`, + `${label}: MCP-owner path emits the MCP query hint; stderr reason gated by GITNEXUS_DEBUG`, () => { const markerPath = path.join(os.tmpdir(), `gitnexus-hook-dbg-${process.pid}-${label}`); const lbugPath = path.join(gitNexusDir, 'lbug'); @@ -1887,7 +1892,9 @@ describe('PreToolUse augmentation filtering (integration)', () => { { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); - expect(result.stdout.trim()).toBe(''); + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); + expect(output!.additionalContext).toContain('validateUser'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped: MCP server owns DB'); expect(fs.existsSync(markerPath)).toBe(false); @@ -1900,13 +1907,13 @@ describe('PreToolUse augmentation filtering (integration)', () => { ); // #1913: the GITNEXUS_DEBUG contract is strict — ONLY '1' and 'true' enable - // diagnostics. Pin that non-canonical truthy-looking values ('0', 'false') - // are treated as OFF, so the skip stays silent. A truthy-gated reader would - // have emitted on these; this guards the unified strict gate (incl. the - // main() catch handler) across the claude/plugin copies. + // the stderr diagnostic. Pin that non-canonical truthy-looking values ('0', + // 'false') are treated as OFF, so stderr stays silent. The #2396 MCP-query + // hint on stdout is independent of GITNEXUS_DEBUG (it is the augmentation, not + // a diagnostic) and must still be emitted here. for (const debugValue of ['0', 'false']) { it.skipIf(SKIP_LSOF_PATH)( - `${label}: MCP-owner skip stays SILENT with GITNEXUS_DEBUG='${debugValue}' (strict contract)`, + `${label}: MCP-owner hint emits on stdout; stderr stays silent with GITNEXUS_DEBUG='${debugValue}'`, () => { const markerPath = path.join( os.tmpdir(), @@ -1933,7 +1940,8 @@ describe('PreToolUse augmentation filtering (integration)', () => { { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: debugValue } }, ); - expect(result.stdout.trim()).toBe(''); + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.stderr.trim()).toBe(''); expect(result.status).toBe(0); expect(fs.existsSync(markerPath)).toBe(false); @@ -1948,6 +1956,143 @@ describe('PreToolUse augmentation filtering (integration)', () => { } }); +// #2396: the owner-path hint is throttled to at most once per repo per window +// (GITNEXUS_MCP_HINT_THROTTLE_MS, default 10min) via a per-repo `.mcp-hint-shown` +// marker, so an owner-locked session isn't nudged on every search. macOS/other- +// Unix lsof+ps lane only (SKIP_LSOF_PATH), like the sibling owner tests. hookEnv +// sets the window to 0 (disabled) elsewhere for determinism; here we set a real +// window to exercise the throttle. +describe.skipIf(SKIP_LSOF_PATH)('MCP-owner hint throttle (#2396)', () => { + for (const [label, hookPath] of [ + ['CJS', CJS_HOOK], + ['Plugin', PLUGIN_HOOK], + ] as const) { + it(`${label}: emits once, then throttles within the window (marker gates it)`, () => { + const markerPath = path.join(os.tmpdir(), `gn-hook-throttle-${process.pid}-${label}`); + const lbugPath = path.join(gitNexusDir, 'lbug'); + const throttleMarker = path.join(gitNexusDir, '.mcp-hint-shown'); + fs.writeFileSync(lbugPath, ''); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(throttleMarker, { force: true }); + const binDir = createHookToolDir({ + gitnexusMarkerPath: markerPath, + lsofOutput: '12345\n', + psOutput: 'node /tmp/node_modules/.bin/gitnexus mcp\n', + }); + const runOnce = () => + runHook( + hookPath, + { + hook_event_name: 'PreToolUse', + tool_name: 'Grep', + tool_input: { pattern: 'validateUser' }, + cwd: tmpDir, + }, + undefined, + { env: { ...hookEnv(binDir), GITNEXUS_MCP_HINT_THROTTLE_MS: '600000' } }, + ); + try { + // First owner-locked search: emits the hint and writes the marker. + const first = runOnce(); + const out1 = parseHookOutput(first.stdout); + expect(out1!.additionalContext).toContain('mcp__gitnexus__query'); + expect(first.status).toBe(0); + expect(fs.existsSync(throttleMarker)).toBe(true); + // Second search, marker still fresh (10-min window): throttled — no hint. + const second = runOnce(); + expect(second.stdout.trim()).toBe(''); + expect(second.status).toBe(0); + } finally { + fs.rmSync(lbugPath, { force: true }); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(throttleMarker, { force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }); + } +}); + +// #2396: buildMcpQueryHint and its throttle are triplicated across the three hook +// copies (the repo's deliberate no-shared-module hook convention). Guard against +// silent drift with a source-level byte-identity check — runs on every platform, +// unlike the owner-path behavior tests which are macOS-only. +describe('hook copy drift guard (#2396)', () => { + const ANTIGRAVITY_HOOK = path.resolve( + __dirname, + '..', + '..', + 'hooks', + 'antigravity', + 'gitnexus-antigravity-hook.cjs', + ); + const HOOK_SOURCES: ReadonlyArray = [ + ['claude', CJS_HOOK], + ['plugin', PLUGIN_HOOK], + ['antigravity', ANTIGRAVITY_HOOK], + ]; + + function extractFn(source: string, name: string): string { + const match = source.match(new RegExp(`function ${name}\\([^)]*\\) \\{[\\s\\S]*?\\n\\}`)); + return match ? match[0] : `<${name} not found>`; + } + + for (const fnName of ['buildMcpQueryHint', 'shouldEmitMcpHint']) { + it(`${fnName} is byte-identical across all three hook copies`, () => { + const [claude, plugin, antigravity] = HOOK_SOURCES.map(([, p]) => + extractFn(fs.readFileSync(p, 'utf-8'), fnName), + ); + expect(claude).toContain(`function ${fnName}`); + expect(plugin).toBe(claude); + expect(antigravity).toBe(claude); + }); + } +}); + +// #2396: an adversarial search pattern (embedded quote + newline) must not break +// the additionalContext JSON envelope — JSON.stringify in the emit path escapes it +// structurally. Owner-path only (macOS/other-Unix lsof+ps lane, SKIP_LSOF_PATH). +describe.skipIf(SKIP_LSOF_PATH)('MCP hint pattern escaping (#2396)', () => { + for (const [label, hookPath] of [ + ['CJS', CJS_HOOK], + ['Plugin', PLUGIN_HOOK], + ] as const) { + it(`${label}: quote+newline pattern stays JSON-safe in additionalContext`, () => { + const markerPath = path.join(os.tmpdir(), `gn-hook-esc-${process.pid}-${label}`); + const lbugPath = path.join(gitNexusDir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + fs.rmSync(markerPath, { force: true }); + const binDir = createHookToolDir({ + gitnexusMarkerPath: markerPath, + lsofOutput: '12345\n', + psOutput: 'node /tmp/node_modules/.bin/gitnexus mcp\n', + }); + const evilPattern = 'foo"bar\nbaz'; + try { + const result = runHook( + hookPath, + { + hook_event_name: 'PreToolUse', + tool_name: 'Grep', + tool_input: { pattern: evilPattern }, + cwd: tmpDir, + }, + undefined, + { env: hookEnv(binDir) }, + ); + // parseHookOutput JSON.parses stdout — a broken envelope would throw/return null. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('foo"bar'); + expect(output!.additionalContext).toContain('search_query'); + expect(result.status).toBe(0); + } finally { + fs.rmSync(lbugPath, { force: true }); + fs.rmSync(markerPath, { force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }); + } +}); + describe.skipIf(SKIP_LSOF_PATH)( 'Ladybug DB owner guard — production-shaped ps + failure modes (#1493)', () => { @@ -1990,7 +2135,11 @@ describe.skipIf(SKIP_LSOF_PATH)( undefined, { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); @@ -2054,7 +2203,11 @@ describe.skipIf(SKIP_LSOF_PATH)( undefined, { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); @@ -2142,7 +2295,11 @@ describe.skipIf(SKIP_LSOF_PATH)( undefined, { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); @@ -2152,11 +2309,13 @@ describe.skipIf(SKIP_LSOF_PATH)( } }); - // #1913: the fail-closed (probe-timeout) skip routes through the SAME gated - // line as the MCP-owner skip, so it too must be silent by default. Symmetric - // counterpart to the debug-on test above, so a regression that ungated the - // ETIMEDOUT path specifically would still be caught. - it(`${label}: ETIMEDOUT lsof → augment skipped SILENTLY by default`, () => { + // #2396/#1913: the fail-closed (probe-timeout) skip routes through the SAME + // owner branch, so it now emits the conditional MCP-query hint on stdout — + // truthful here because the hint only asks the agent to use the MCP tools + // "if they are live". The stderr diagnostic stays debug-gated (empty by + // default), so strict runners still see no unexpected diagnostic. Symmetric + // counterpart to the debug-on test above. + it(`${label}: ETIMEDOUT lsof → emits hint on stdout, stderr silent by default`, () => { const markerPath = path.join(os.tmpdir(), `gn-hook-etime-silent-${process.pid}-${label}`); const lbugPath = path.join(gitNexusDir, 'lbug'); fs.writeFileSync(lbugPath, ''); @@ -2178,7 +2337,8 @@ describe.skipIf(SKIP_LSOF_PATH)( undefined, { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '' } }, ); - expect(result.stdout.trim()).toBe(''); + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.stderr.trim()).toBe(''); expect(result.status).toBe(0); expect(fs.existsSync(markerPath)).toBe(false); @@ -2226,7 +2386,11 @@ describe.skipIf(SKIP_LSOF_PATH)( }, }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); @@ -2279,7 +2443,11 @@ describe.skipIf(SKIP_LSOF_PATH)( }, }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); @@ -2335,7 +2503,11 @@ describe.skipIf(SKIP_LSOF_PATH)( }, }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); @@ -2391,7 +2563,11 @@ describe.skipIf(SKIP_LSOF_PATH)( }, }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); @@ -2516,7 +2692,11 @@ describe.skipIf(SKIP_LSOF_PATH)( undefined, { env: { ...hookEnv(binDir), GITNEXUS_DEBUG: '1' } }, ); - expect(result.stdout.trim()).toBe(''); + // #2396: owner path now hands the agent the MCP-query hint on stdout; + // the CLI augment is still skipped (marker absent) and the stderr + // skip diagnostic remains debug-gated. + const output = parseHookOutput(result.stdout); + expect(output!.additionalContext).toContain('mcp__gitnexus__query'); expect(result.status).toBe(0); expect(result.stderr).toContain('[GitNexus] augment skipped'); expect(fs.existsSync(markerPath)).toBe(false); diff --git a/gitnexus/test/utils/hook-test-helpers.ts b/gitnexus/test/utils/hook-test-helpers.ts index f5203a023..a7838971a 100644 --- a/gitnexus/test/utils/hook-test-helpers.ts +++ b/gitnexus/test/utils/hook-test-helpers.ts @@ -220,6 +220,10 @@ export function hookEnv(binDir: string) { GITNEXUS_HOOK_CLI_PATH: path.join(binDir, 'gitnexus-cli.js'), GITNEXUS_HOOK_LSOF_PATH: path.join(binDir, 'lsof'), GITNEXUS_HOOK_PS_PATH: path.join(binDir, 'ps'), + // #2396: disable the per-repo MCP-hint throttle by default so owner tests + // are deterministic (gitNexusDir is shared across the suite). The throttle + // itself is covered by its own dedicated test, which sets a real window. + GITNEXUS_MCP_HINT_THROTTLE_MS: '0', }; }