From 848ed2a0b638235d68fcc53b4940a72ebdde66f0 Mon Sep 17 00:00:00 2001 From: joseph02 Date: Sat, 22 Aug 2026 20:06:45 -0700 Subject: [PATCH] fix(factory-plugin): pin CLI version and parse quoted shell patterns - Pin mcp.json and the hook's npx fallback to gitnexus@ from the plugin manifest, registered with the release sync script so a mutable @latest can never execute on MCP connect or augment fallback - Port the #2938 shell tokenizer (tokenizeShellWords + parseRgGrepPattern) so quoted, backslash-escaped, --regexp=, -eVALUE, and -- patterns survive - Add the #2938 regression matrix and pin assertions to factory-plugin.test.ts --- .../hooks/gitnexus-hook.js | 257 ++++++++++++------ gitnexus-factory-plugin/mcp.json | 2 +- gitnexus/scripts/sync-plugin-manifests.mjs | 8 + gitnexus/test/unit/factory-plugin.test.ts | 46 ++++ .../test/unit/sync-plugin-manifests.test.ts | 22 +- 5 files changed, 238 insertions(+), 97 deletions(-) diff --git a/gitnexus-factory-plugin/hooks/gitnexus-hook.js b/gitnexus-factory-plugin/hooks/gitnexus-hook.js index 6bae2d588..e57bae9cb 100644 --- a/gitnexus-factory-plugin/hooks/gitnexus-hook.js +++ b/gitnexus-factory-plugin/hooks/gitnexus-hook.js @@ -1,23 +1,17 @@ #!/usr/bin/env node /** - * GitNexus Factory AI (Droid) Plugin Hook + * GitNexus Factory AI (Droid) plugin hook. * - * PostToolUse — augments Grep/Glob/Execute searches with graph context from - * the GitNexus index and returns it via hookSpecificOutput.additionalContext. + * PostToolUse — augments Grep/Glob/Execute searches with graph context and + * returns it via hookSpecificOutput.additionalContext. * - * Reuses the same guards as the Claude/Codex adapter (bundled byte-identical, - * kept in lockstep by test/unit/factory-plugin.test.ts): - * - acquireHookSlot — per-repo cap on concurrent augment children so - * parallel sessions can't fan out unbounded `gitnexus augment` spawns - * (#1486). - * - LadybugDB owner probe — skips the CLI augment when a GitNexus MCP/serve - * process already holds the single-writer DB lock, avoiding contention - * (#2396); the in-session MCP tools cover augmentation instead. + * Reuses the Claude adapter's guards, bundled byte-identical: acquireHookSlot + * caps concurrent augment children per repo (#1486), and the LadybugDB owner + * probe skips the CLI augment when an MCP/serve process already holds the + * single-writer lock (#2396). * - * The augment CLI child is NOT wrapped in the coreutils `timeout` - * orphan-containment guard the full Claude adapter uses (#2163) — same scope - * as the Cursor integration. Add it (resolveUnixGuardTimeout is exported by - * the bundled probe) if orphaned augment children become a problem here. + * The augment child is not wrapped in the coreutils `timeout` orphan guard the + * full Claude adapter uses (#2163) — same scope as the Cursor integration. */ const fs = require('fs'); @@ -26,22 +20,22 @@ const { spawnSync } = require('child_process'); const { acquireHookSlot } = require('./hook-lock.js'); const { hasGitNexusDbLockedByGitNexusServer } = require('./hook-db-lock-probe.cjs'); -/** - * Read JSON input from stdin synchronously. - */ +// Pin the CLI instead of tracking `latest`: npm versions are immutable, so only +// a plugin revision can change what the fallback below executes. The release +// stamps this manifest (gitnexus/scripts/sync-plugin-manifests.mjs). +const { version: PINNED_VERSION } = require('../.factory-plugin/plugin.json'); + function readInput() { try { - const data = fs.readFileSync(0, 'utf-8'); - return JSON.parse(data); + return JSON.parse(fs.readFileSync(0, 'utf-8')); } catch { return {}; } } /** - * A `.gitnexus/` that holds `registry.json`/`repos` (and no per-repo index - * metadata) is the global registry, not a repo index — never augment against - * it. Mirrors the Claude adapter's guard. + * A `.gitnexus/` holding `registry.json`/`repos` (and no per-repo index + * metadata) is the global registry, not a repo index — never augment against it. */ function isGlobalRegistryDir(candidate) { if ( @@ -56,10 +50,7 @@ function isGlobalRegistryDir(candidate) { ); } -/** - * Walk up from startDir looking for a non-registry `.gitnexus/` folder. Returns - * the path to `.gitnexus/` or null if not found within 5 levels. - */ +/** Walk up from startDir for a non-registry `.gitnexus/`, at most 5 levels. */ function findGitNexusDir(startDir) { let dir = startDir || process.cwd(); for (let i = 0; i < 5; i++) { @@ -73,9 +64,138 @@ function findGitNexusDir(startDir) { } /** - * Extract a search pattern from a Factory tool payload. Factory's shell tool is - * `Execute` (Claude's is `Bash`); Grep/Glob match Claude's. + * Split a command the way a POSIX shell would, so quoted and backslash-escaped + * patterns survive as one token. Kept identical to the Cursor adapter's + * tokenizer (#2938) so the two can collapse into a shared module later. */ +function tokenizeShellWords(command) { + const tokens = []; + let current = ''; + let quote = null; + let escaped = false; + let hasToken = false; + + for (let index = 0; index < command.length; index += 1) { + const char = command[index]; + if (escaped) { + current += char; + escaped = false; + hasToken = true; + continue; + } + + if (quote === "'") { + if (char === "'") quote = null; + else current += char; + hasToken = true; + continue; + } + + if (quote === '"') { + if (char === '"') { + quote = null; + } else if (char === '\\') { + const next = command[index + 1]; + // Inside double quotes a backslash only escapes these four; otherwise + // it is a literal character (so Windows paths survive intact). + if (next === '$' || next === '`' || next === '"' || next === '\\') { + escaped = true; + } else { + current += '\\'; + } + } else { + current += char; + } + hasToken = true; + continue; + } + + if (char === '\\') { + escaped = true; + hasToken = true; + } else if (char === "'" || char === '"') { + quote = char; + hasToken = true; + } else if (/\s/.test(char)) { + if (hasToken) tokens.push(current); + current = ''; + hasToken = false; + } else { + current += char; + hasToken = true; + } + } + + if (escaped) current += '\\'; + if (hasToken) tokens.push(current); + return tokens; +} + +/** Recover the search pattern from an `rg`/`grep` command line. */ +function parseRgGrepPattern(cmd) { + const tokens = tokenizeShellWords(cmd); + let foundCmd = false; + let skipNext = false; + let skipNextAsPattern = false; + let endOfOptions = false; + const flagsWithValues = new Set([ + '-e', + '-f', + '-m', + '-A', + '-B', + '-C', + '-g', + '--glob', + '-t', + '--type', + '--include', + '--exclude', + ]); + const patternFlags = new Set(['-e', '--regexp']); + + for (const token of tokens) { + if (skipNext) { + skipNext = false; + if (skipNextAsPattern) { + return token.length >= 3 ? token : null; + } + continue; + } + if (!foundCmd) { + // Match on the basename so absolute paths (`/usr/bin/rg`) and Windows + // `rg.exe` count as the command. + const commandName = token + .split(/[\\/]/) + .pop() + ?.replace(/\.exe$/i, ''); + if (commandName === 'rg' || commandName === 'grep') foundCmd = true; + continue; + } + if (endOfOptions) { + return token.length >= 3 ? token : null; + } + if (token === '--') { + endOfOptions = true; + continue; + } + if (token.startsWith('-')) { + const attachedPattern = token.match(/^--regexp=(.+)$/) || token.match(/^-e(.+)$/); + if (attachedPattern) { + return attachedPattern[1].length >= 3 ? attachedPattern[1] : null; + } + if (flagsWithValues.has(token) || patternFlags.has(token)) { + skipNext = true; + skipNextAsPattern = patternFlags.has(token); + } + continue; + } + return token.length >= 3 ? token : null; + } + return null; +} + +/** Factory's shell tool is `Execute` (Claude's is `Bash`); Grep/Glob match Claude's. */ function extractPattern(toolName, toolInput) { if (toolName === 'Grep') { return toolInput.pattern || null; @@ -90,67 +210,24 @@ function extractPattern(toolName, toolInput) { if (toolName === 'Execute') { const cmd = toolInput.command || ''; if (!/\brg\b|\bgrep\b/.test(cmd)) return null; - - // NOTE: split(/\s+/) cannot handle shell quoting, same as the Cursor - // integration. `rg "User Service" src/` yields "User" (first token after - // rg/grep, quotes stripped) rather than the full phrase — BM25 is already - // token-tolerant, so the multi-word pattern is deliberately not - // reconstructed. Worst case is augment context for a narrower term than - // the agent searched. Quoted single tokens (`rg "validateUser"`) are exact. - const tokens = cmd.split(/\s+/); - let foundCmd = false; - let skipNext = false; - const flagsWithValues = new Set([ - '-e', - '-f', - '-m', - '-A', - '-B', - '-C', - '-g', - '--glob', - '-t', - '--type', - '--include', - '--exclude', - ]); - - for (const token of tokens) { - if (skipNext) { - skipNext = false; - continue; - } - if (!foundCmd) { - if (/\brg\b|\bgrep\b/.test(token)) foundCmd = true; - continue; - } - if (token.startsWith('-')) { - if (flagsWithValues.has(token)) skipNext = true; - continue; - } - const cleaned = token.replace(/['"]/g, ''); - return cleaned.length >= 3 ? cleaned : null; - } - return null; + return parseRgGrepPattern(cmd); } return null; } /** - * Run `gitnexus augment` for `pattern` and return its stderr (the augment CLI - * writes results to stderr; LadybugDB's native module captures stdout at the OS - * fd level, making it unusable in subprocess contexts). Tries a PATH-installed - * binary first, then falls back to npx. + * Run `gitnexus augment` for `pattern` and return its stderr — the augment CLI + * writes results there because LadybugDB's native module captures stdout at the + * OS fd level. * - * Honors GITNEXUS_HOOK_CLI_PATH first, same as the Claude adapter: it runs the - * CLI as `node `, which is the only branch that works on Windows, where - * Node refuses to spawn the `.cmd` launcher shims below without a shell - * (CVE-2024-27980). Falls back to a PATH binary, then npx. + * GITNEXUS_HOOK_CLI_PATH is tried first and run as `node `, the only form + * that works on Windows, where Node refuses to spawn the `.cmd` shims without a + * shell (CVE-2024-27980). Then a PATH binary, then a version-pinned npx. * - * SECURITY: `pattern` is passed after the `--` end-of-options marker and never - * through a shell — the Windows npx fallback invokes `npx.cmd` directly rather - * than `shell: true`, so a pattern like `-rf` or `$(...)` is inert. + * SECURITY: `pattern` follows the `--` end-of-options marker and never reaches a + * shell (the Windows fallback invokes `npx.cmd` directly rather than + * `shell: true`), so `-rf` or `$(...)` is inert. */ function runAugment(pattern, cwd) { const isWin = process.platform === 'win32'; @@ -186,7 +263,11 @@ function runAugment(pattern, cwd) { } try { - const child = spawnSync(isWin ? 'npx.cmd' : 'npx', ['-y', 'gitnexus', ...args], spawnOpts); + const child = spawnSync( + isWin ? 'npx.cmd' : 'npx', + ['-y', `gitnexus@${PINNED_VERSION}`, ...args], + spawnOpts, + ); if (!child.error && child.status === 0 && child.stderr && child.stderr.trim()) { return child.stderr; } @@ -219,9 +300,9 @@ function main() { let result = ''; try { if (hasGitNexusDbLockedByGitNexusServer(path.join(gitNexusDir, 'lbug'), process.pid)) { - // #2396: a GitNexus MCP/serve process owns the single-writer DB, so a - // competing CLI augment would only contend on the lock. The session's - // MCP tools cover augmentation instead — skip silently. + // #2396: an MCP/serve process owns the single-writer DB, so a competing + // CLI augment would only contend on the lock. Its MCP tools cover + // augmentation instead — skip silently. return; } result = runAugment(pattern, cwd); @@ -246,4 +327,6 @@ function main() { } } -main(); +if (require.main === module) main(); + +module.exports = { parseRgGrepPattern, tokenizeShellWords }; diff --git a/gitnexus-factory-plugin/mcp.json b/gitnexus-factory-plugin/mcp.json index cd02d4285..4fe6590dd 100644 --- a/gitnexus-factory-plugin/mcp.json +++ b/gitnexus-factory-plugin/mcp.json @@ -2,7 +2,7 @@ "mcpServers": { "gitnexus": { "command": "npx", - "args": ["-y", "gitnexus@latest", "mcp"] + "args": ["-y", "gitnexus@1.6.9", "mcp"] } } } diff --git a/gitnexus/scripts/sync-plugin-manifests.mjs b/gitnexus/scripts/sync-plugin-manifests.mjs index 5bcc8f29c..44ca89f92 100644 --- a/gitnexus/scripts/sync-plugin-manifests.mjs +++ b/gitnexus/scripts/sync-plugin-manifests.mjs @@ -13,6 +13,8 @@ * - gitnexus-claude-plugin/.codex-plugin/plugin.json (top-level version) * - .agents/plugins/marketplace.json (plugins[gitnexus]) * - gitnexus-claude-plugin/skills//mcp.json (gitnexus@ launch arg, x10) + * - gitnexus-factory-plugin/.factory-plugin/plugin.json (top-level version) + * - gitnexus-factory-plugin/mcp.json (gitnexus@ launch arg) * * Modes: * node scripts/sync-plugin-manifests.mjs rewrite stale surfaces @@ -54,6 +56,12 @@ const MANIFEST_SURFACES = [ file: `gitnexus-claude-plugin/skills/${name}/mcp.json`, kind: 'mcp', })), + // The Factory plugin's manifest is also its pin source at runtime: the + // PostToolUse hook reads this version to build its `npx -y gitnexus@` + // fallback, so stamping it here keeps the hook and the MCP entry on the same + // released CLI. + { file: 'gitnexus-factory-plugin/.factory-plugin/plugin.json', kind: 'plugin' }, + { file: 'gitnexus-factory-plugin/mcp.json', kind: 'mcp' }, ]; const PLUGIN_NAME = 'gitnexus'; diff --git a/gitnexus/test/unit/factory-plugin.test.ts b/gitnexus/test/unit/factory-plugin.test.ts index 3a62b628a..990342853 100644 --- a/gitnexus/test/unit/factory-plugin.test.ts +++ b/gitnexus/test/unit/factory-plugin.test.ts @@ -16,6 +16,7 @@ */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { spawnSync } from 'child_process'; +import { createRequire } from 'module'; import fs from 'fs'; import path from 'path'; import os from 'os'; @@ -38,6 +39,11 @@ const CLAUDE_HOOKS = path.join(REPO_ROOT, 'gitnexus-claude-plugin', 'hooks'); // canonical Claude-adapter copies. const BUNDLED_GUARDS = ['hook-lock.js', 'hook-db-lock-probe.cjs', 'win-rm-list-json.ps1'] as const; +const require_ = createRequire(import.meta.url); +const { parseRgGrepPattern } = require_(HOOK) as { + parseRgGrepPattern: (command: string) => string | null; +}; + // ─── Manifest / file presence ─────────────────────────────────────── describe('Factory plugin files', () => { @@ -96,6 +102,16 @@ describe('Factory mcp.json', () => { expect(mcp.mcpServers?.gitnexus?.command).toBe('npx'); expect(mcp.mcpServers.gitnexus.args).toContain('mcp'); }); + + // Executed state, not quickstart docs: `@latest` would let a future registry + // upload run on MCP connect without a plugin revision. + it('pins the CLI to the released version instead of a mutable tag', () => { + const pkg = JSON.parse( + fs.readFileSync(path.join(REPO_ROOT, 'gitnexus', 'package.json'), 'utf-8'), + ); + expect(mcp.mcpServers.gitnexus.args).toContain(`gitnexus@${pkg.version}`); + expect(JSON.stringify(mcp)).not.toContain('gitnexus@latest'); + }); }); // ─── hooks.json wiring ────────────────────────────────────────────── @@ -155,6 +171,14 @@ describe('Factory hook source regressions', () => { expect(source).toContain('npx.cmd'); }); + // The npx fallback runs on ordinary tool calls whenever the CLI is not on + // PATH, so an unpinned ref would execute whatever currently owns the tag. + it('pins the npx fallback to the manifest version, never a mutable tag', () => { + expect(source).not.toContain('gitnexus@latest'); + expect(source).toContain("require('../.factory-plugin/plugin.json')"); + expect(source).toContain('`gitnexus@${PINNED_VERSION}`'); + }); + // Windows regression: Node refuses to spawn the .cmd launcher shims without a // shell (CVE-2024-27980), so `node ` is the only branch that runs // there. Same escape hatch the Claude adapter honors. @@ -182,6 +206,28 @@ describe('Factory hook source regressions', () => { }); }); +// ─── Execute pattern parsing (shares the Cursor adapter's #2938 matrix) ─ + +describe('Factory Execute pattern parser', () => { + it.each([ + ['rg "User Service" src/', 'User Service'], + ["grep 'error boundary' -- src/", 'error boundary'], + ['rg User\\ Service src/', 'User Service'], + [String.raw`rg "C:\Users" src/`, String.raw`C:\Users`], + ['rg -e "User Service" src/', 'User Service'], + ['rg --regexp=UserService src/', 'UserService'], + ['grep -eUserService src/', 'UserService'], + ['/usr/bin/rg -- "User Service" src/', 'User Service'], + ['rg -- -error src/', '-error'], + ['rg "validateUser"', 'validateUser'], + ['rg -t ts UserService src/', 'UserService'], + ['rg --glob "*.ts" UserService', 'UserService'], + ['rg ab src/', null], + ])('extracts %j from %j', (command, expected) => { + expect(parseRgGrepPattern(command)).toBe(expected); + }); +}); + // ─── Behavior: early-exit paths (no augment spawned) ──────────────── describe('Factory hook behavior — early exits', () => { diff --git a/gitnexus/test/unit/sync-plugin-manifests.test.ts b/gitnexus/test/unit/sync-plugin-manifests.test.ts index e932bad4a..b750e879d 100644 --- a/gitnexus/test/unit/sync-plugin-manifests.test.ts +++ b/gitnexus/test/unit/sync-plugin-manifests.test.ts @@ -9,8 +9,11 @@ const SURFACES = [ '.claude-plugin/marketplace.json', 'gitnexus-claude-plugin/.codex-plugin/plugin.json', '.agents/plugins/marketplace.json', + 'gitnexus-factory-plugin/.factory-plugin/plugin.json', ] as const; +const FACTORY_MCP = 'gitnexus-factory-plugin/mcp.json'; + const MCP_SKILL_DIRS = [ 'gitnexus-plan', 'gitnexus-work', @@ -24,7 +27,7 @@ const MCP_SKILL_DIRS = [ 'gitnexus-refactoring', ] as const; -const TOTAL_SURFACES = SURFACES.length + MCP_SKILL_DIRS.length; +const TOTAL_SURFACES = SURFACES.length + MCP_SKILL_DIRS.length + 1; function mcpPath(dir: string): string { return `gitnexus-claude-plugin/skills/${dir}/mcp.json`; @@ -56,8 +59,9 @@ function makeRoot(packageVersion: string, manifestVersion: string): string { name: 'gitnexus-marketplace', plugins: [{ name: 'gitnexus', version: manifestVersion, category: 'Developer Tools' }], }); - for (const dir of MCP_SKILL_DIRS) { - writeJson(root, mcpPath(dir), { + writeJson(root, SURFACES[4], { name: 'gitnexus', version: manifestVersion }); + for (const dir of [...MCP_SKILL_DIRS.map(mcpPath), FACTORY_MCP]) { + writeJson(root, dir, { mcpServers: { gitnexus: { command: 'npx', args: ['-y', `gitnexus@${manifestVersion}`, 'mcp'] }, }, @@ -89,19 +93,19 @@ describe('syncPluginManifests (#2445)', () => { expect(result.version).toBe('1.6.10-rc.29'); expect(result.synced).toHaveLength(TOTAL_SURFACES); expect(result.stale.map(({ from }) => from)).toEqual(Array(TOTAL_SURFACES).fill('1.6.9')); - expect(readVersions(root)).toEqual(Array(4).fill('1.6.10-rc.29')); + expect(readVersions(root)).toEqual(Array(SURFACES.length).fill('1.6.10-rc.29')); }); - it('pins the gitnexus@ launch arg in every plugin skill mcp.json', () => { + it('pins the gitnexus@ launch arg in every executable mcp.json', () => { const root = makeRoot('1.6.10-rc.29', '1.6.9'); syncPluginManifests(root); - for (const dir of MCP_SKILL_DIRS) { - const mcp = JSON.parse(readFileSync(path.join(root, mcpPath(dir)), 'utf8')) as { + for (const file of [...MCP_SKILL_DIRS.map(mcpPath), FACTORY_MCP]) { + const mcp = JSON.parse(readFileSync(path.join(root, file), 'utf8')) as { mcpServers: { gitnexus: { args: string[] } }; }; - expect(mcp.mcpServers.gitnexus.args).toEqual(['-y', 'gitnexus@1.6.10-rc.29', 'mcp']); + expect(mcp.mcpServers.gitnexus.args, file).toEqual(['-y', 'gitnexus@1.6.10-rc.29', 'mcp']); } }); @@ -131,7 +135,7 @@ describe('syncPluginManifests (#2445)', () => { expect(result.stale).toHaveLength(TOTAL_SURFACES); expect(result.synced).toHaveLength(0); - expect(readVersions(root)).toEqual(Array(4).fill('1.6.9')); + expect(readVersions(root)).toEqual(Array(SURFACES.length).fill('1.6.9')); }); it('changes only the version text and preserves the surrounding formatting', () => {